fix: don't fail nodes on artifact plugin sidecar exit codes

[Joibel]

• Ran make pre-commit -B (scoped: golangci-lint run workflow/controller/ — only a pre-existing ns_watcher.go SA1019 unrelated to this change; full go test ./workflow/controller/ green)
• Signed-off commits with Conventional Commit messages
• PR title is a conventional commit message (it becomes the release notes entry)
• Unit or e2e tests cover the change
• For features: an associated issue and a feature description file (make feature-new) — not a feature
• Opened as draft; will mark "Ready for review" once builds are green

Motivation

TestExampleWorkflows/../../examples/artifact-passing-explicit-plugin.yaml fails intermittently in CI (observed twice within two days, on both the legacy and init-less pod layouts): the workflow's pod completes successfully end to end, yet the node fails with artifact-plugin-test: Error (exit code 2).

Investigation with the CI log archives and a local reproduction rig established the mechanism:

1. The wait (or supervisor) container finishes saving all outputs and logs, then tears down the artifact plugin sidecar via the file-signal mechanism; the sidecar's log shows a clean, complete shutdown ending with "artifact plugin sidecar command exited".
2. The controller's terminateContainers sweep also SIGTERMs the sidecar via a kubectl exec … argoexec kill 15 1 session. In both CI failures that exec completed within 6ms/80ms of the sidecar's final log line — i.e. it was in flight at the instant PID 1 exited.
3. When an exec session races container PID 1's exit, the container runtime can record a phantom non-zero exit status for a process that exited cleanly. Reproduced locally in plain docker (no Kubernetes): 3 phantom exit 2 results in 100 attempts of file-signal teardown with kill execs spammed at the dying container, each with a byte-identical clean log and Error: "" in the runtime state. No panic is involved — the wrapper genuinely exits 0.
4. kubelet reports the phantom exit code, the pod phase becomes Failed, and the node fails.

Modifications

inferFailedReason now ignores non-zero exit codes from artifact plugin sidecar containers (artifact-plugin-*), letting the existing main/wait verdict at the end of the function decide the node's fate. Rationale: the aux container tears these sidecars down only after all saves succeeded, so an aux container that exited 0 proves the sidecar did its job, and its exit code carries no information about the node's outcome — while demonstrably being unreliable under the exec race. This is the same position Kubernetes native sidecar containers take: their exit status does not affect pod phase.

Safety is preserved by the function's existing structure: a plugin sidecar that genuinely dies mid-save fails the aux container's save, and the wait container did not complete successfully verdict fails the node. Failures of main, wait/supervisor, init containers, and user sidecars are unaffected (covered by tests).

Both teardown mechanisms (aux file-signal and controller SIGTERM sweep) are deliberately left in place.

Verification

New table-driven TestInferFailedReasonArtifactPluginSidecar: plugin-sidecar exit ignored (legacy wait + init-less supervisor + SIGTERM 143 variants), and negative cases proving main/wait/user-sidecar failures still fail the node with unchanged messages. The three ignore cases fail on main and pass with the fix. Full go test ./workflow/controller/ green.

The container-runtime misattribution itself (exec racing PID 1 exit) is arguably a containerd/runc issue; a standalone docker reproducer exists and an upstream report may follow separately.

Documentation

Not needed: no user-visible behavior change beyond removing spurious failures.

AI

This PR was prepared with Claude Code (Anthropic): CI log forensics, local docker reproduction of the runtime race, fix, tests, and this description, directed and reviewed by the submitting maintainer.

🤖 Generated with Claude Code

https://claude.ai/code/session_018XsqqvLSrJ5sH8gN8tLbr9

Summary by CodeRabbit

• Bug Fixes

  • Prevented artifact plugin shutdown exit codes from incorrectly marking otherwise successful workflows as failed.
  • Workflow failures from the main, wait, or regular sidecar containers continue to be reported normally.

• Tests

  • Added coverage for artifact plugin sidecars across legacy, initless, and signal-terminated executions.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: d1587206-a97f-4f16-9e7c-31fcec8cb33d

📥 Commits

Reviewing files that changed from the base of the PR and between faf6c5c and b4bee14.

📒 Files selected for processing (2)

• workflow/controller/operator.go
• workflow/controller/operator_test.go

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

---

📝 Walkthrough

Walkthrough

The controller now ignores artifact plugin sidecar exit codes when inferring node failure reasons. Tests cover artifact plugin exits and confirm that wait, main, and user-sidecar failures remain reported.

Changes

Artifact plugin sidecar handling

Layer / File(s)	Summary
Failure reason inference and validation workflow/controller/operator.go, workflow/controller/operator_test.go	inferFailedReason skips artifact plugin sidecars after logging their exit codes. Tests cover exit code 2, SIGTERM, initless execution, and failures from wait, main, and user-sidecar containers.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to b4bee

This localized controller change ignores artifact-plugin sidecar exit codes while preserving failures from main, wait, and user-sidecar containers; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: shuangkun, isubasinghe, nitinmoningi

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main fix: preventing artifact plugin sidecar exit codes from failing nodes.
Description check	✅ Passed	The description covers motivation, modifications, verification, documentation, AI use, checklist items, and non-feature status.
Docstring Coverage	✅ Passed	Docstring check was indeterminate for this PR — some files could not be analyzed in time. Not blocking.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}