Fix all unresolved CodeRabbit comments on this PR:

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

internal/config/cnimgr/cnimgr.go (2)

94-99: ⚠️ Potential issue | 🔴 Critical

Treat watcher channels as terminal after readiness/shutdown.

AddWatcher() currently appends when lastError != nil, including errShutdown (Line 127), so callers can get parked forever after shutdown. Also, watcher lists are not cleared after notifications (Lines 94-99 and 150-155), so stale channels are revisited repeatedly.

Suggested fix

 func (c *CNIManager) AddWatcher() chan bool {
 	c.mutex.Lock()
 	defer c.mutex.Unlock()

 	watcher := make(chan bool, 1)
-	if c.lastError == nil {
+	switch {
+	case c.lastError == nil:
 		watcher <- true
-	} else {
+		close(watcher)
+	case errors.Is(c.lastError, errShutdown):
+		watcher <- false
+		close(watcher)
+	default:
 		c.watchers = append(c.watchers, watcher)
 	}

 	return watcher
 }

 		for _, watcher := range c.watchers {
 			select {
 			case watcher <- true:
 			default:
 			}
+			close(watcher)
 		}
+		c.watchers = nil
 	}

 	for _, watcher := range c.watchers {
 		select {
 		case watcher <- false:
 		default:
 		}
+		close(watcher)
 	}
+	c.watchers = nil
 }

Also applies to: 122-133, 136-155

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/config/cnimgr/cnimgr.go` around lines 94 - 99, The watcher channels
are treated as persistent but must be terminal: update AddWatcher() to NOT
append new watchers when c.lastError != nil (including errShutdown) so callers
don't get parked after shutdown, and change the notification logic in the loop
that iterates c.watchers (and the similar block around lines 136-155) to treat
channels as one‑time: send the readiness/shutdown signal non-blockingly to each
watcher (select { case watcher <- true: default: }), then remove/clear that
watcher from the slice (or set c.watchers = nil) so stale channels are not
revisited; ensure this uses the existing symbols AddWatcher, c.watchers,
c.lastError and errShutdown so behavior is consistent across readiness and
shutdown paths.

---

72-76: ⚠️ Potential issue | 🟠 Major

Use context-aware CNI status checks (and avoid status call under the lock).

At Line 76, c.plugin.Status() ignores the provided ctx. If status blocks, it can hold the write lock longer than necessary and stall ReadyOrError(), AddWatcher(), and Shutdown() paths.

Suggested fix

 func (c *CNIManager) statusPollFunc(ctx context.Context, stopOnReady bool) (bool, error) {
-	c.mutex.Lock()
-	defer c.mutex.Unlock()
-
-	if err := c.plugin.Status(); err != nil {
+	if err := c.plugin.StatusWithContext(ctx); err != nil {
+		c.mutex.Lock()
 		c.lastError = err
+		c.mutex.Unlock()
 		return false, nil
 	}
 
+	c.mutex.Lock()
+	defer c.mutex.Unlock()
+
 	if c.lastError != nil {
 		c.lastError = nil

#!/bin/bash
set -euo pipefail
# Verify ocicni interface methods and current cnimgr usage.
rg -n -C2 'type CNIPlugin interface|StatusWithContext\(|Status\(' \
  vendor/github.com/cri-o/ocicni/pkg/ocicni/types.go \
  internal/config/cnimgr/cnimgr.go

As per coding guidelines, "Propagate context.Context through function calls in Go code".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/config/cnimgr/cnimgr.go` around lines 72 - 76, The statusPollFunc
currently calls c.plugin.Status() while holding c.mutex and without passing ctx;
change it to avoid calling the blocking status under the lock and to use a
context-aware API: grab a local reference to the plugin (e.g., p := c.plugin)
while holding c.mutex, release the lock, then call the context-aware status
method (preferably p.StatusWithContext(ctx) if available, or add/use a
StatusWithContext wrapper on the CNI plugin) and only re-acquire the lock to
update manager state based on the result; update statusPollFunc, and ensure
callers like ReadyOrError(), AddWatcher(), and Shutdown() are not blocked by the
status call.

internal/config/cnimgr/cnimgr.go (1)

88-90: Prefer structured logrus fields for GC error logging.

At Line 89, use structured logging instead of Warnf so error data is consistently queryable.

Suggested refactor

-			logrus.Warnf("Garbage collect stale network resources failed: %v", err)
+			logrus.WithError(err).Warn("Garbage collect stale network resources failed")

As per coding guidelines, "Use logrus with structured fields for logging in Go code".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/config/cnimgr/cnimgr.go` around lines 88 - 90, Replace the
unstructured Warnf call that logs GC errors with a structured logrus entry: when
c.doGC(ctx) returns an error, call logrus.WithError(err).Warn("Garbage collect
stale network resources failed") (or use
logrus.WithFields(logrus.Fields{"error": err}).Warn(...)) so the c.doGC failure
is logged using structured fields; update the block that currently uses
logrus.Warnf to use logrus.WithError(err).Warn and keep the existing message
text.

internal/config/cnimgr/cnimgr.go (1)

72-100: ⚠️ Potential issue | 🟠 Major

Follow-up: external CNI work still runs under c.mutex.

statusPollFunc keeps the write lock held across StatusWithContext, validPodList(), and plugin.GC(). If any of those block, ReadyOrError() on the kubelet status path (server/runtime_status.go:1-20) and watcher registration in server/sandbox_network.go:367-378 stall behind the poller. Please narrow the critical section to state transitions and watcher handoff, and run the external work outside the mutex.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/config/cnimgr/cnimgr.go` around lines 72 - 100, statusPollFunc
currently holds c.mutex while calling external/blocking work
(plugin.StatusWithContext, validPodList/doGC), causing other paths to stall;
change it so the mutex is only held to read/update internal state and to hand
off watchers. Concretely: acquire c.mutex to snapshot any required state (e.g.
c.lastError, copy of c.watchers) and then release the mutex before calling
plugin.StatusWithContext, validPodList(), or doGC; perform those calls and
compute the new state outside the lock, then re-acquire c.mutex to update
c.lastError, clear/replace c.watchers and perform the non-blocking watcher
sends. Ensure you use local variables for results and only mutate CNIManager
fields (c.lastError, c.watchers) while holding c.mutex to keep transitions
atomic.

internal/config/cnimgr/cnimgr.go (2)

41-49: Pass a parent context into the poller.

The manager now owns a long-lived goroutine, but it is rooted in context.Background(), so its lifetime is detached from the caller and Shutdown() becomes the only cancellation path. Please accept a parent context.Context here and derive the internal cancelable context from that instead.

As per coding guidelines, "Propagate context.Context through function calls in Go code".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/config/cnimgr/cnimgr.go` around lines 41 - 49, The poller is created
from context.Background() so its lifetime is detached; change the constructor
that creates CNIManager to accept a parent context (e.g. ctx context.Context)
and derive the cancellable context with context.WithCancel(parentCtx) instead of
context.Background(), then pass that derived ctx into mgr.pollContinuously(ctx);
keep the existing mgr.cancel and Shutdown() behavior so Shutdown still calls
mgr.cancel() but ensure callers supply the parent context when creating the
CNIManager (update the function signature that instantiates CNIManager
accordingly).

---

88-90: Log GC failures as structured fields.

Please keep the error as a field here rather than formatting it into the message.

📝 Suggested change

-			logrus.Warnf("Garbage collect stale network resources failed: %v", err)
+			logrus.WithError(err).Warn("Garbage collect stale network resources failed")

As per coding guidelines, "Use logrus with structured fields for logging in Go code".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/config/cnimgr/cnimgr.go` around lines 88 - 90, The log currently
formats the error into the message using logrus.Warnf after calling c.doGC(ctx);
change it to use structured logging so the error is attached as a field (e.g.,
use logrus.WithError(err).Warn(...) or logrus.WithField("err", err).Warn(...))
and keep the original descriptive text "Garbage collect stale network resources
failed" as the message; update the error handling where c.doGC(ctx) is called
and replace the logrus.Warnf invocation accordingly.

Fix all unresolved CodeRabbit comments on this PR:

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

internal/config/cnimgr/cnimgr_test.go (1)

62-72: ⚠️ Potential issue | 🟡 Minor

newTestManager still races startup readiness in GC tests.

Because polling starts immediately, subtests that assign mgr.validPodList after construction can miss the first ready transition and intermittently skip the GC call they assert on.

💡 Deterministic helper pattern

-func newTestManager(plugin *fakeCNIPlugin) *CNIManager {
+func newTestManager(plugin *fakeCNIPlugin, preStart func(*CNIManager)) *CNIManager {
 	ctx, cancel := context.WithCancel(context.Background())
 	mgr := &CNIManager{
 		plugin:         plugin,
 		lastError:      errors.New("plugin status uninitialized"),
 		cancel:         cancel,
 		initPollInterval:    testPollInterval,
 		monitorPollInterval: testPollInterval,
 	}
+	if preStart != nil {
+		preStart(mgr)
+	}
 	go mgr.pollContinuously(ctx)
 	return mgr
}

Then GC tests can install validPodList in preStart before polling begins.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/config/cnimgr/cnimgr_test.go` around lines 62 - 72, newTestManager
starts the pollContinuously goroutine immediately which races tests that want to
set mgr.validPodList before polling begins; change newTestManager to not start
polling itself (remove the go mgr.pollContinuously(ctx) call) and instead
provide a deterministic start hook (e.g., add a Start/BeginPolling method or
expose PollContinuouslyStart(ctx) that calls mgr.pollContinuously) so tests can
set mgr.validPodList in their preStart setup and then invoke mgr.Start(ctx) to
begin polling; update tests to call the new start method after installing
validPodList and keep references to pollContinuously and validPodList to locate
the affected code.

internal/config/cnimgr/cnimgr.go (1)

89-90: Use structured logrus fields for error logging.

Replace logrus.Warnf() with logrus.WithError(err).Warn() to align with structured logging conventions and enable better queryability in logs.

Suggested change

-		if err := c.doGC(ctx); err != nil {
-			logrus.Warnf("Garbage collect stale network resources failed: %v", err)
-		}
+		if err := c.doGC(ctx); err != nil {
+			logrus.WithError(err).Warn("Garbage collect stale network resources failed")
+		}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/config/cnimgr/cnimgr.go` around lines 89 - 90, Replace the
unstructured log call logrus.Warnf("Garbage collect stale network resources
failed: %v", err) with a structured log using the error field: use
logrus.WithError(err).Warn("Garbage collect stale network resources failed") so
the same message is logged but the error is attached as structured data; update
the occurrence where logrus.Warnf is called and ensure the err variable is
passed into WithError.