argoproj / argoproj/argo-workflows

ErrDeadlineExceeded from the retry swap-back path corrupts an already-Succeeded node to Error, failing successful workflows

Open
#16,638 2 comments 1 reaction 0 assignees View on GitHub
Dominant language
Go
Stars
17k
Forks
3.7k
Avg merge
1d 15h
Merged PRs (30d)
138

Description

### Pre-requisites

- [x] I have double-checked my configuration
- [x] I have searched existing issues and could not find a match for this bug
- [x] I have "tested against `:latest`" **by line-verifying the source rather than running it** — see "On the `:latest` requirement" below, which explains this in detail.
- [ ] I'd like to contribute the fix myself

## What happened? What did you expect to happen?

A DAG node that had **already been marked `Succeeded`** was overwritten to `Error` with message `Deadline exceeded`, causing the whole `Workflow` to report `status.phase: Error`. Every pod in the workflow exited 0 and all work completed successfully. This produced a false failure alert and paged an on-call team for a successful run.

**Expected:** a controller-side reconcile timeout should cause a clean bail-out and requeue, never a mutation of a completed node's phase.

### This is structurally guaranteed, not a race

In `workflow/controller/operator.go`, the "Swap the node back to retry node" block re-enters `executeTemplate` for the retry parent. That re-entry is gated on the child already being fulfilled:

```go
// v3.6.5:2262 (main:2575)
if !retryNode.Fulfilled() && node.Fulfilled() {
retryNode, err = woc.executeTemplate(ctx, retryNodeName, orgTmpl, tmplCtx, args, opts)
if err != nil {
// v3.6.5:2266 (main:2577)
return woc.markNodeError(node.Name, err), err
}
}
```

`node.Fulfilled() == true` is therefore an invariant at the `markNodeError` call. If that re-entry returns `ErrDeadlineExceeded`, a completed node is **always** corrupted. There is no timing window in which this is safe.

The `ErrDeadlineExceeded` comes from the per-reconcile budget:

```go
// v3.6.5:133 (main:148)
maxOperationTime = envutil.LookupEnvDurationOr("MAX_OPERATION_TIME", 30*time.Second)
// v3.6.5:165
deadline: time.Now().UTC().Add(maxOperationTime),
// v3.6.5:1974-1977 (main:2247-2250)
if time.Now().UTC().After(woc.deadline) {
woc.log.Warnf("Deadline exceeded")
woc.requeue()
return node, ErrDeadlineExceeded
}
```

### This is the one `ErrDeadlineExceeded` path missing the guard from #3921

#3905 / #3921 established the rule that `ErrDeadlineExceeded` must abort cleanly rather than mutate node state, adding `case ErrDeadlineExceeded: return` to `dag.go` and `steps.go`. On `main`, `ErrDeadlineExceeded` is handled at four sites — `operator.go:409`, `operator.go:495`, `steps.go:300`, `dag.go:670` — all of which bail out without touching phase.

`operator.go:2577` is the **only** site that converts it into a node phase change. The swap-back error path was introduced in #1696 / #1892, roughly a year before #3921's deadline discipline, and was never revisited.

### `markNodePhase`'s "already fulfilled" check is a log, not a guard

```go
// v3.6.5:2647-2654 (main:3080-3086)
if node.Phase != phase {
if node.Phase.Fulfilled() {
woc.log.WithFields(...).Error("node is already fulfilled")
}
woc.log.Infof("node %s phase %s -> %s", node.ID, node.Phase, phase)
node.Phase = phase // applied regardless
woc.updated = true
}
```

Introduced log-only in #3949 (which downgraded a `panic` to a log for the workflow-level check and added this node-level check as a warning from the start). A terminal node's phase should arguably be immutable absent an explicit retry/resubmit.

### Why it then fails the whole workflow

With `retryPolicy: OnFailure`, the corrupted `Error` child is not eligible for retry, so the retry parent inherits `Error` and the workflow ends `Error`:

```
Retry Policy: OnFailure (onFailed: true, onError false)
Node not set to be retried after status: Error
```

### Preconditions

Only two, both trivial:

1. Any `retryStrategy` in the spec — the swap-back block is gated on `retryNodeName != ""` (v3.6.5:2254).
2. One reconcile exceeding `MAX_OPERATION_TIME` (default 30s, unchanged since #4562 made it configurable in 2020).

On our production cluster ~5,600 of ~3.2M reconciles per week exceed the 30s budget (0.18%, from `argo_workflows_operation_duration_seconds_bucket`). Of 1,567 `CronWorkflow`s, 281 declare a `retryStrategy` and are therefore exposed.

### On the `:latest` requirement

I have not run `:latest`. Instead I line-verified all four code sites across **v3.6.5, v3.6.6, v3.6.10, v3.6.19, v3.7.0, v3.7.9, v3.7.17, v4.1.0 and `main`**:

| site | v3.6.5 | v4.1.0 / main | changed? |
|---|---|---|---|
| `maxOperationTime` default | 133 | 148 | no — `30*time.Second` throughout |
| `Warn("Deadline exceeded")` + sentinel return | 1975 / 1977 | 2248 / 2250 | no |
| `// Swap the node back to retry node` | 2253 | 2564 | no |
| `return woc.markNodeError(node.Name, err), err` | 2266 | 2577 | no — still unconditional |
| `Error("node is already fulfilled")` | 2650 | 3083 | no — still log-then-apply |

The only drift is cosmetic: the `slog` refactor (#14527), `errors.Is()` instead of `switch err`, `Fulfilled()` → `Phase.Fulfilled(node.TaskResultSynced)`, and an added daemon-node branch inside the swap-back block. No behavioural change at any site. I mention this to pre-empt an "upgrade to the latest patch" triage response.

### Related

- **#3905 / #3921** — the rule this path violates. I'd frame a fix as completing #3921 rather than as new design.
- **#16485** — same class of bug, maintainer-accepted framing: *"This is backpressure, not a terminal failure… the node was marked `Error` instead of being requeued."*
- **#15034** — same family, inverse symptom (stuck `Running` rather than corrupted `Error`).
- **#16396**, **#15740** — recent work on not clobbering `NodeSucceeded` / correctly phasing retry wrapper nodes.
- **#13044 / #13049** — adjacent deadline territory but a different mechanism; noting it to pre-empt duplicate closure.

### Suggested fix

Add the #3921 guard to the swap-back path so a deadline bail-out never mutates phase, e.g.:

```go
if !retryNode.Fulfilled() && node.Fulfilled() {
retryNode, err = woc.executeTemplate(ctx, retryNodeName, orgTmpl, tmplCtx, args, opts)
if err != nil {
if errors.Is(err, ErrDeadlineExceeded) {
return node, err // requeue; do not corrupt the fulfilled child
}
return woc.markNodeError(ctx, node.Name, err), err
}
}
```

Optionally also make `markNodePhase` refuse a fulfilled→different transition rather than logging and proceeding.

## Version(s)

v3.6.5 — and line-verified as present in v3.6.6, v3.6.10, v3.6.19, v3.7.0, v3.7.9, v3.7.17, v4.1.0 and `main` (see table above).

## Paste a minimal workflow that reproduces the issue.

⚠️ **Transparency: I have not executed this exact manifest.** It is derived from the code path plus a production incident whose logs are below. Our real workflow uses a private image, so I cannot paste it. The reliable way to force precondition (2) is to shrink the reconcile budget so effectively every reconcile exceeds it:

```bash
kubectl -n argo set env deploy/workflow-controller MAX_OPERATION_TIME=1ms
```

Then submit a DAG with a workflow-level `retryStrategy` and a step that succeeds. Expected corrupt outcome: a `Succeeded` node flipped to `Error`, workflow `phase: Error`, with `node is already fulfilled fromPhase=Succeeded toPhase=Error` in the controller log.

```yaml
apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
generateName: deadline-corrupts-succeeded-node-
spec:
entrypoint: main
# Any retryStrategy is enough: it makes nodes retry nodes, which is what
# gates the "Swap the node back to retry node" block (v3.6.5:2254).
retryStrategy:
limit: 1
retryPolicy: "OnFailure"
templates:
- name: main
dag:
tasks:
- name: a
template: echo
- name: b
template: echo
dependencies: [a]
- name: echo
container:
image: alpine:3.20
command: [sh, -c]
args: ["echo ok"]
```

If a maintainer confirms this shape is the repro they want, I'm happy to run it against `:latest` in an isolated cluster and post the resulting controller log.

## Logs from the workflow controller

Redacted production logs (workflow and node names genericised; timestamps and ordering verbatim). This is a single reconcile — the next `Processing workflow` for this object is at `07:22:58.100`.

```text
07:22:09.149 level=info msg="Processing workflow" Phase=Running ResourceVersion=... workflow=wf-abc123
07:22:09.151 level=info msg="node changed" new.phase=Succeeded old.phase=Running nodeID=wf-abc123-1533255685
07:22:33.615 level=info msg="node wf-abc123-1363776902 phase Running -> Succeeded"
07:22:41.765 level=info msg="Lock has been released by .../wf-abc123-1363776902. Available locks: 1" mutex=.../Mutex/shared-lock
07:22:41.765 level=info msg="node wf-abc123-3529216299 phase Running -> Succeeded"
07:22:41.765 level=info msg="node wf-abc123-3529216299 finished: 2026-08-11 07:22:41 +0000 UTC"
07:22:41.765 level=warning msg="Deadline exceeded" workflow=wf-abc123
07:22:41.765 level=error msg="Mark error node" error="Deadline exceeded" nodeName="wf-abc123(0).step-b(0)"
07:22:41.765 level=error msg="node is already fulfilled" fromPhase=Succeeded toPhase=Error nodeName="wf-abc123(0).step-b(0)"
07:22:41.765 level=info msg="node wf-abc123-3529216299 phase Succeeded -> Error"
07:22:41.765 level=info msg="node wf-abc123-3529216299 message: Deadline exceeded"
07:22:58.100 level=info msg="Processing workflow" Phase=Running ResourceVersion=... <- next reconcile
07:23:44.915 level=info msg="Retry Policy: OnFailure (onFailed: true, onError false)"
07:23:44.915 level=info msg="Node not set to be retried after status: Error"
07:23:44.915 level=info msg="node wf-abc123-3895466584 phase Running -> Error"
07:23:44.915 level=info msg="node wf-abc123-3895466584 message: Deadline exceeded"
```

Elapsed from `Processing workflow` (07:22:09.149) to `Deadline exceeded` (07:22:41.765) is **32.6s**, against the 30s default budget.

Resulting node tree — note every `Pod` node is `Succeeded` with `exitCode: 0`, and only the composite nodes carry `Deadline exceeded`:

```text
NODE TYPE PHASE EXITCODE MESSAGE
step-a(0) Pod Succeeded 0
step-b(0) Pod Succeeded 0
exit-handler(0) Pod Succeeded 0
step-b Retry Succeeded 0
step-b-dag(0) DAG Error - Deadline exceeded <- corrupted
step-b-dag Retry Error - Deadline exceeded
wf-abc123(0) DAG Error -
```

## Logs from in your workflow's wait container

Not informative for this bug, and included to make that point: the wait containers all report clean completion. Every pod exited 0; nothing failed at the pod level.

```text
time="2026-08-11T06:57:07.832Z" level=info msg="sub-process exited" argo=true error=""
time="2026-08-11T07:21:58.133Z" level=info msg="sub-process exited" argo=true error=""
```

The corruption is entirely controller-side and post-completion, which is why it cannot be diagnosed from pod or wait-container logs.

Contributor guide

Open the contributing guide

Research direction

Start in workflow/controller/operator.go at the retry swap-back block and compare its ErrDeadlineExceeded handling with the guards in dag.go and steps.go. Reproduce with MAX_OPERATION_TIME=1ms and the supplied workflow, then verify that a deadline during reconciliation requeues without changing an already-Succeeded node to Error.

Written by the indexing model from the issue text.

Assessment

Tech stack
go, kubernetes
Domain
backend, infrastructure
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.