Skip to content

fix(k8s): fail execs whose stream closes without a status - #449

Open
mitja-kleider wants to merge 1 commit into
actions:mainfrom
mitja-kleider:k8s-exec-close-without-status
Open

mitja-kleider wants to merge 1 commit into
actions:mainfrom
mitja-kleider:k8s-exec-close-without-status

Conversation

@mitja-kleider

Copy link
Copy Markdown

Part of #396.

When the exec WebSocket closes before Kubernetes sends a status frame, the runner marked the step as succeeded while the command was still running. This rejects in that case instead, marking the step as failed.

A sturdier design would have the hook's wrapper script write its exit code to a file in the job pod. On a dropped stream, the hook could open a new exec and poll that file until the code appears. The step would then finish with its real result instead of failing. That's a bigger change than this PR.

@chris-thomas-dev

Copy link
Copy Markdown

Confirmed in production, with a trace of the failure mode this fixes

We hit the exact failure this PR targets on 2026-10-08, running hooks v0.8.1 (as bundled in actions/runner 2.337.0) under ARC in Kubernetes mode on EKS. A deploy step was cut off part-way, the job went green, and only one of six packages was deployed. Posting the evidence here since the issue (#396) had a reproducer but no field occurrence with logs.

What happened

  • EKS rotated control-plane instances while the step was running. The cluster audit log shows system:apiserver rewriting the default/kubernetes Endpoints at 05:57:31, :34 and :41 as instances left and joined.
  • The step's RunScriptStep exec (received 05:57:12) was served by one of the departing API server instances. In the audit log it has a ResponseStarted entry and never gets a ResponseComplete. Every other exec in the window completed normally, including the post-step ones on the same pod seconds later, served by a new instance.
  • On the node, containerd logged ttrpc: received message on inactive stream at 05:57:38.542, then Failed to pipe "stdout"/"stderr" for container exec …: use of closed network connection, and kubelet later failed to kill exec …: context canceled. So the kubelet side saw the client connection vanish while the command was still running.
  • The runner's worker trace shows the hook process for RunScriptStep finishing at 05:57:38.55 with exit code 0 and an empty error list, 7 seconds after the last line of step output. The step was recorded as succeeded.
  • The command kept running in the job pod until the runner's cleanup deleted the pod about 2 seconds later. It was mid docker buildx build, and BuildKit logged the orphaned session dying with cancelled contexts.

So the sequence is: websocket closed cleanly by the API server, no status frame, execPodStep's promise never settles, the event loop drains, Node exits 0. The ws.once('close') rejection in this PR would have turned that into a failed step.

Two things worth considering alongside this

  1. The same never-settle shape exists in internalExecOutput, the output-hash exec and execCpToPod/execCpFromPod. runScriptStep calls execCpFromPod right after the step to copy _runner_file_commands back, so a drop there would still exit 0 and silently lose outputs. A general guard in the entrypoint would cover all of them: default process.exitCode = 1 and set 0 only on explicit success, or a beforeExit handler that fails if run() hasn't returned. That makes "pending promise plus drained loop" unable to produce a green step anywhere in the hook.
  2. A test for "close after the status frame is a no-op" would protect against a regression into false failures, since that's the path every healthy step takes.

Happy to share the full audit, journal and runner log excerpts if useful. Thanks for the fix.


Disclaimer: this comment was drafted with Claude (Anthropic) from our incident investigation, then reviewed before posting.

@amanfredi

Copy link
Copy Markdown

Claude (an AI assistant) here, posting on behalf of @amanfredi.

This PR fixes the hang in execPodStep. The tar-copy and hash helpers have the same gap, though, so a step can still go green without its script ever running.

execCpToPod, execCpFromPod and execCalculateOutputHashSorted each wrap exec.exec in their own promise. Like execPodStep before this PR, those promises settle only when a status frame arrives. If the stream drops during one of them, the promise never settles and the hook exits 0.

We tested by running the released v0.8.1 bundle as run_script_step against a local fake API server. The fake answers every exec with a Success status except one, whose TCP connection it resets instead. Rows are in run order:

Dropped exec v0.8.1 v0.8.1 + this PR v0.8.1 + fix in Exec.exec
mkdir exit 0, script never runs exit 1 exit 1
copy in exit 0, script never runs exit 0, script never runs copy retried, script runs
hash check after copy in exit 0, script never runs exit 0, script never runs check retried, script runs
merge exit 0, script never runs exit 1 exit 1
script exit 0 exit 1 exit 1
copy out exit 0, file commands not copied back exit 0, file commands not copied back copy retried, exit 0
hash check after copy out exit 0 exit 0 check retried, exit 0

A clean close frame instead of a reset gives the same results.

In production (ARC kubernetes-novolume on EKS, runner 2.338.2, hooks v0.8.1), a deploy step had been silent for 21 minutes when it ended as success mid-output. It logged no execPodStep response: line, skipped the copy back from the workflow pod, and job cleanup then killed the command. ARC and node-autoscaler logs show nothing at that time. That's consistent with the API-server replacement trigger described in #396.

What we run now is a patch to the bundled client-node Exec.exec, which every hook exec goes through. If the socket closes with no status, it writes exec stream closed before the command reported an exit status to the exec's stderr and passes { status: 'Failure', message } to the status callback. execPodStep rejects on that status. The copy helpers ignore the status but reject on stderr output, so their retry loops re-copy, and the hash checks, which run inside the helpers' verification loop, are recomputed. Alternatively, this PR could extend its guard to the three helpers. Either fix needs to cover them.

A review and release would help us drop the patch. Thanks @mitja-kleider for the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants