fix: create artifact plugin socket dir before starting the plugin server. Fixes #16784

[myzk-a]

See the pull request guide for details on each item.

• Ran make pre-commit -B
• 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)
• Opened as draft; will mark "Ready for review" once builds are green

Fixes #16784

Motivation

In the artifact-plugin init container (init-artifact-<name>), argoexec forks the plugin server before creating the directory the server binds its unix socket in. When the forked server wins that race, bind() fails with ENOENT, the server exits, and nothing notices — argoexec then waits out its full 120-second socket timeout while the pod sits in Init:1/2, and the step fails.

This affects any workflow that loads an artifact through a plugin. It is also the cause of the intermittent test-examples CI failures on examples/artifact-passing-explicit-plugin.yaml, which reproduce at 2/54 = 3.7% of suite runs (see #16784 for the measurement).

Only the load path is affected. The sidecar used to save artifacts gets an emptyDir mounted at exactly SocketDir(), so kubelet creates the directory and the ordering cannot matter there. The init container has no such mount and relies solely on os.MkdirAll — which is why only init-artifact-* ever hangs.

Modifications

Move os.MkdirAll(pluginName.SocketDir(), 0755) out of loadArtifactPlugin and up above the go func() that starts the plugin server, so the directory is guaranteed to exist before the server can call bind().

This does not introduce a new invariant: it makes the init path match the ordering the sidecar path has always had (directory exists → server starts → bind succeeds).

Behaviour is otherwise unchanged. The only observable difference outside the success path is that a MkdirAll failure is now reported before the server is forked rather than after, so no orphaned child process is created, and the error carries context.

I have deliberately left the second half of the problem for a follow-up, and would rather agree the shape of it on #16784 first: when the plugin server dies for any reason, argoexec still waits the full 120 s for a socket that can never appear, because startCommand returns successfully and nothing then watches the child. NewDriver's retry loop already selects on ctx.Done(), so cancelling the context when the child exits would be enough — but the cleanest way to observe that exit interacts with closer() (which is itself cmd.Wait()), so it deserves its own discussion.

Verification

Reproduced deterministically by delaying only the MkdirAll — a single time.Sleep(500 * time.Millisecond) at the top of loadArtifactPlugin, not committed — which makes the forked server reach bind() first every time:

	jobs / suite runs	failed	bind: no such file or directory
unfixed + delay	10	10	10
fixed + delay	30	0	0

The only difference between the two runs is the position of MkdirAll; the delay is identical in both, at the same place in the source. Without the delay the failure occurs naturally in 2/54 suite runs.

Every failure in the unfixed run was on artifact-passing-explicit-plugin.yaml, with the same error and the same ~130 s duration as the failures that occur naturally, so the delay does not create a new failure mode — it makes the existing one certain.

The captured container log of a failure (with the controller at --loglevel=debug, which the executor inherits):

level=DEBUG msg="starting command" name=/artifact-server args=[/tmp/artifact-plugins/test/socket]
{"level":"ERROR","msg":"Failed to start server",
 "error":"listen unix /tmp/artifact-plugins/test/socket: bind: no such file or directory"}
level=DEBUG msg="plugin socket not found, retrying in 1s" retry=0   maxRetries=120
                          ... 120 retry lines, one per second ...
level=DEBUG msg="plugin socket not found, retrying in 1s" retry=119 maxRetries=120
level=ERROR msg="executor error" error="failed to create plugin driver for test: plugin test
  expected unix socket at \"/tmp/artifact-plugins/test/socket\" but it does not exist after
  waiting for 120 seconds"

and the same container after the fix:

level=DEBUG msg="starting command" name=/artifact-server
{"level":"INFO","msg":"Unix socket created successfully","mode":"Srwxr-xr-x"}
{"level":"INFO","msg":"Server ready to accept connections"}
level=INFO  msg="plugin socket file exists and is a unix socket" mode=Srwxr-xr-x
level=INFO  msg="Load artifact" artifactName=message duration=21.884018ms
level=INFO  msg="Successfully download file"

Runs, on my fork:

• natural failure rate: https://github.com/myzk-a/argo-workflows/actions/runs/32198377586
• forced race, unfixed (10/10 fail): https://github.com/myzk-a/argo-workflows/actions/runs/32318400471
• forced race, fixed (30/30 pass): https://github.com/myzk-a/argo-workflows/actions/runs/32320008624

On tests: the race is not deterministically testable in a unit test, and I did not want to add one that passes whether or not the bug is present. The existing e2e coverage (test-examples / examples/artifact-passing-explicit-plugin.yaml) exercises this path on every CI run — it is what surfaced the bug — and the two runs above are the evidence that the ordering is the cause. Happy to add something else if reviewers would prefer it.

One thing worth flagging separately: none of this was visible in CI, because .github/actions/e2e-failure-debug/action.yml collects pod logs with a single kubectl logs --all-containers, which aborts at the first not-yet-started container and therefore drops every line from the container holding the answer. I will send that as a separate PR; it is useful regardless of this fix.

Documentation

No documentation change. This is an internal bug fix with no user-visible behaviour change — the documented plugin contract (the server listens on the socket path it is given) is unchanged; this makes argo hold up its own end of it reliably.

AI

Claude Code was used throughout this investigation: analysing the CI logs, identifying the root cause, designing the forced-race experiment above, and drafting #16784 and this description.

The code change is mine, and I reviewed and understand it. I ran every CI experiment on my fork myself and read the resulting logs. Before proposing the change I had the history of #14915 checked to confirm the current ordering was not a deliberate design decision that I would be undoing — it was never discussed there, which is why I am confident this is a placement oversight rather than a fence worth leaving standing.

Summary by CodeRabbit

• Bug Fixes
  • Artifact plugins now reliably create their socket directory before starting.
  • Improved error reporting when the socket directory cannot be created.
  • Enhanced consistency when loading the initialized plugin.

[myzk-a]

Marking this ready for review. All checks are green except one, which is unrelated to this change.

The failing check is E2E Tests (test-functional, minimal, false, true) — TestRetrySuite/TestWorkflowTemplateWithRetryStrategyInContainerSet. It is the fail-fast behaviour #16000 addresses: podHasContainerNeedingTermination returns true as soon as any main container exits non-zero, so when c2 fails to start, cleanUpPod terminates the whole pod before c3 gets to run its retry. From the pod log in this run:

c3  23:54:30.458  capturing logs                    <- attempt 1
c3               intentional failure               <- printed once
c2  23:54:30.345  Error: exec: "invalid": executable file not found in $PATH
    23:54:30.718  queueing pod for cleanup  action=terminateContainers
    23:54:30.878  signaled container  container=c3  stdout="killing 1 with terminated"
c3  23:54:30.873  file signal handler exiting due to context cancellation
c3  23:54:31.458  capturing logs                    <- attempt 2, exactly 1.000s later
c3  23:54:31.458  sub-process exited  error="failed to start command: context canceled"

capturing logs appears twice and intentional failure once, which is exactly the assertion that fails (retry_test.go:157, expected 2, actual 1). The same test fails the same way on main (https://github.com/argoproj/argo-workflows/actions/runs/32233028127), on the legacy layout rather than init-less, so it is not specific to either.

This PR touches only cmd/argoexec/commands/artifact_plugin_init.go, i.e. the artifact-plugin-init command, which runs solely in init-artifact-* containers and only when a template has an input artifact backed by a plugin. The failing test uses no artifact plugins. Both test-examples jobs — where the bug in #16784 reproduces — are green.

Happy to rebase once #16000 lands if you would rather see a fully green run first.

[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: 4a415297-ef20-4ee1-b15b-b77e93a79e93

📥 Commits

Reviewing files that changed from the base of the PR and between 4d06b91 and 2296144.

📒 Files selected for processing (1)

• cmd/argoexec/commands/artifact_plugin_init.go

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

---

📝 Walkthrough

Walkthrough

The artifact plugin initialization command now creates the socket directory before starting the plugin server. It passes the existing plugin name to loadArtifactPlugin, which no longer creates the directory.

Changes

Artifact plugin initialization

Layer / File(s)	Summary
Prepare socket directory before plugin startup cmd/argoexec/commands/artifact_plugin_init.go	The command creates the socket directory with 0755 permissions and wraps creation errors. It reuses the constructed pluginName when loading the plugin. loadArtifactPlugin no longer creates the socket directory.

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

Merge Risk: ⚪ Minimal · up to 22961

The change creates the artifact-plugin socket directory before starting the server, preventing the reported startup race without changing the plugin contract; no actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: joibel, isubasinghe

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR fixes the socket-directory race but does not implement the linked issue's requirement to detect server failures and stop waiting immediately [#16784].	Implement plugin-server exit monitoring and cancel the socket wait, or separate that requirement into a clearly linked follow-up issue.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 1 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly describes the primary fix: creating the artifact plugin socket directory before starting the server.
Description check	✅ Passed	The description covers motivation, modifications, verification, documentation, AI use, issue linkage, and required checklist items.
Out of Scope Changes check	✅ Passed	The code change is limited to creating the socket directory before starting the plugin server and matches the linked issue scope.

✨ 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.}