internal/oci/runtime_oci.go (1)

1217-1231: Consider releasing opLock before waiting for exit file in the error path.

In this error path, waitForExitFile can block for up to ~5 seconds while holding c.opLock. This differs from the pattern at lines 1260-1262 where the lock is explicitly released before waiting.

Holding the lock during the wait could:

1. Block concurrent ContainerStatus CRI calls (context snippet 2)
2. Block ContainerStateToDisk operations (context snippet 4)
3. Delay pod status checks in Kubernetes

However, since the container state is already being set to ContainerStateStopped (line 1216) before the wait, releasing the lock could expose partially updated state. The current approach prioritizes consistency over concurrency.

💡 Optional: Release lock before waiting (mirrors lines 1260-1262 pattern)

If concurrency is preferred over atomicity here, consider:

 			c.state.Status = ContainerStateStopped
 
-			// The exit file may not exist yet if conmon hasn't written it.
-			// Wait for it to appear before defaulting to exit code 255.
-			// This prevents a race where fast-exiting containers
-			// get a spurious exit code 255.
-			waitErr := waitForExitFile(c)
+			c.opLock.Unlock()
+			waitErr := waitForExitFile(c)
+			c.opLock.Lock()
+
 			if waitErr != nil {

This would require careful review of the state consistency implications.

🤖 Prompt for AI Agents

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

In `@internal/oci/runtime_oci.go` around lines 1217 - 1231, The waitForExitFile
call holds c.opLock for up to ~5s causing unwanted blocking; change the
error-path to release the lock before waiting and re-acquire it before mutating
container state—i.e., unlock c.opLock (matching the unlock pattern used around
lines 1260–1262), call waitForExitFile(c), then lock c.opLock again and only
then set c.state.Finished and c.state.ExitCode or call
updateContainerStatusFromExitFile(c); ensure you re-acquire the same lock used
elsewhere so state updates remain synchronized and no other goroutine can
concurrently mutate c.state.

internal/oci/runtime_oci_test.go (2)

263-263: Defer cleanup before mock setup to ensure cleanup on test failure.

The defer cmdrunner.ResetPrependedCmd() statements (lines 263 and 323) are placed after cmdrunner.SetMocked(runner). If any setup between SetMocked and defer fails, the mock won't be reset. Consider moving the defer immediately after setting the mock.

💡 Move defer immediately after SetMocked

 		runner := runnerMock.NewMockCommandRunner(mockCtrl)
 		cmdrunner.SetMocked(runner)
-
 		defer cmdrunner.ResetPrependedCmd()
+
 		runner.EXPECT().Command(gomock.Any(), gomock.Any()).Return(

Also applies to: 323-323

🤖 Prompt for AI Agents

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

In `@internal/oci/runtime_oci_test.go` at line 263, Move each defer
cmdrunner.ResetPrependedCmd() to immediately follow the corresponding
cmdrunner.SetMocked(runner) call so the mock is always reset even if subsequent
setup fails; locate the two occurrences where SetMocked(runner) is called in
internal/oci/runtime_oci_test.go and place defer cmdrunner.ResetPrependedCmd()
directly after each SetMocked invocation (before any further setup or
assertions) to ensure cleanup on test failure.

---

238-298: Good test coverage for the fix. Minor suggestions for robustness.

The test correctly validates that UpdateContainerStatus waits for the exit file and reads exit code 0.

Two minor suggestions:

1. Use filepath.Join for path construction (line 274): Consistent with how exitFilePath() constructs the path in production code.

2. Potential test flakiness: The 800ms delay relies on the backoff timing. If the first retry happens faster than expected (e.g., due to scheduling), the test could still pass; if slower, it could flake. Consider using a channel or file watcher to signal readiness instead of a fixed sleep.

💡 Use filepath.Join for consistency

+		exitFilePath := filepath.Join(containerDir, "exit")
+
 		// Write the exit file after a short delay, simulating conmon
 		// writing it after the first read attempt fails.
 		go func() {
 			time.Sleep(800 * time.Millisecond)
 			Expect(os.WriteFile(
-				containerDir+"/exit", []byte("0"), 0o644,
+				exitFilePath, []byte("0"), 0o644,
 			)).To(Succeed())
 		}()

🤖 Prompt for AI Agents

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

In `@internal/oci/runtime_oci_test.go` around lines 238 - 298, Replace the
hard-coded path construction containerDir+"/exit" with
filepath.Join(containerDir, "exit") to match exitFilePath() and avoid OS path
issues, and remove the fixed time.Sleep delay: instead use a signaling channel
to deterministically control when the test goroutine writes the exit file
(create a chan struct{}, have the goroutine block on receiving from it before
calling os.WriteFile(filepath.Join(containerDir, "exit"), ...), and signal the
channel from the test at the appropriate point around the call to
runtime.UpdateContainerStatus); refer to variables and functions containerDir,
os.WriteFile, filepath.Join, runtime.UpdateContainerStatus and sut.State() to
locate where to change the test.

internal/oci/runtime_oci.go (1)

1166-1186: Consider accepting context.Context for cancellation support.

The function can block for up to ~5 seconds without being cancellable. If the upstream context is cancelled (e.g., during shutdown), this function will continue polling. While the existing file operations in this codebase follow similar patterns, adding context support would improve responsiveness.

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

♻️ Optional: Add context support using `kwait.ExponentialBackoffWithContext`

-// waitForExitFile waits for the container's exit file to appear using
-// exponential backoff. Returns nil if the file appeared, or an error if it did
-// not appear within the timeout (~5s total: 500ms initial, 1.2 factor, 6 steps).
-func waitForExitFile(c *Container) error {
+// waitForExitFile waits for the container's exit file to appear using
+// exponential backoff. Returns nil if the file appeared, or an error if it did
+// not appear within the timeout (~5s total: 500ms initial, 1.2 factor, 6 steps).
+func waitForExitFile(ctx context.Context, c *Container) error {
 	exitFilePath := c.exitFilePath()
 
-	return kwait.ExponentialBackoff(
+	return kwait.ExponentialBackoffWithContext(
+		ctx,
 		kwait.Backoff{
 			Duration: 500 * time.Millisecond,
 			Factor:   1.2,
 			Steps:    6,
 		},
 		func() (bool, error) {

Note: This would require updating call sites to pass context.

🤖 Prompt for AI Agents

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

In `@internal/oci/runtime_oci.go` around lines 1166 - 1186, Update waitForExitFile
to accept a context.Context and use kwait.ExponentialBackoffWithContext so the
polling can be cancelled; specifically change the signature of waitForExitFile(c
*Container) error to waitForExitFile(ctx context.Context, c *Container) error,
replace kwait.ExponentialBackoff(...) with
kwait.ExponentialBackoffWithContext(ctx, ...), and propagate the context through
the lambda that calls os.Stat(exitFilePath). Finally, update all call sites of
waitForExitFile to pass the appropriate context (e.g., the upstream context or
context.Background() where cancellation is not available).

internal/oci/runtime_oci.go (1)

1283-1303: Consider accepting context.Context for cancellation support.

The function doesn't currently support cancellation. While kwait.ExponentialBackoff doesn't natively support context, you could use kwait.ExponentialBackoffWithContext or check ctx.Done() within the condition function to allow callers to abort the wait early. This would align with the coding guideline to propagate context through function calls.

That said, given the bounded ~5s maximum wait and the primary goal of fixing the race condition, this is a nice-to-have improvement rather than a blocker.

🔧 Optional: Add context support

-func waitForExitFile(c *Container) error {
+func waitForExitFile(ctx context.Context, c *Container) error {
 	exitFilePath := c.exitFilePath()
 
 	return kwait.ExponentialBackoff(
 		kwait.Backoff{
 			Duration: 500 * time.Millisecond,
 			Factor:   1.2,
 			Steps:    6,
 		},
 		func() (bool, error) {
+			select {
+			case <-ctx.Done():
+				return false, ctx.Err()
+			default:
+			}
 			_, err := os.Stat(exitFilePath)
 			if err != nil {
 				return false, nil
 			}
 
 			return true, nil
 		})
 }

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/oci/runtime_oci.go` around lines 1283 - 1303, The waitForExitFile
function lacks context cancellation support; update its signature to accept ctx
context.Context and use kwait.ExponentialBackoffWithContext (or pass a condition
that checks ctx.Done()) so callers can cancel the wait early: change func
waitForExitFile(c *Container) error to func waitForExitFile(ctx context.Context,
c *Container) error, pass ctx into the backoff call (or return early when ctx is
done inside the closure), and ensure all callers are updated to supply a
context.

internal/oci/runtime_oci_test.go (1)

281-286: Use filepath.Join for constructing the exit file path.

While string concatenation works on Linux, filepath.Join is the idiomatic approach for path construction in Go.

🔧 Suggested change

+		exitFile := filepath.Join(containerDir, "exit")
 		go func() {
 			time.Sleep(800 * time.Millisecond)
 			Expect(os.WriteFile(
-				containerDir+"/exit", []byte("0"), 0o644,
+				exitFile, []byte("0"), 0o644,
 			)).To(Succeed())
 		}()

🤖 Prompt for AI Agents

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

In `@internal/oci/runtime_oci_test.go` around lines 281 - 286, Replace the
string-concatenated path containerDir+"/exit" with filepath.Join(containerDir,
"exit") in the goroutine that writes the exit file (the anonymous goroutine in
runtime_oci_test.go), and add an import for "path/filepath" if it's not already
imported so path construction is platform-idiomatic.