argoproj / argoproj/argo-workflows

controller: Archived label patch can target a retried or recreated workflow

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

Description

## Problem

`archiveWorkflowAux` first writes a workflow snapshot to the archive database, then marks the Kubernetes object as `Archived` with an unconditional merge patch addressed only by namespace and name:

```go
if err := wfc.wfArchive.ArchiveWorkflow(ctx, wf); err != nil {
return fmt.Errorf("failed to archive workflow: %w", err)
}

_, err = wfc.wfclientset.ArgoprojV1alpha1().
Workflows(un.GetNamespace()).
Patch(
ctx,
un.GetName(),
types.MergePatchType,
data,
metav1.PatchOptions{},
)
```

The archive decision is made from an informer object, while the patch is applied to whichever object currently owns that namespace and name at the API server. The patch has no UID or state precondition.

As a result, the archived snapshot and the object receiving the `Archived` label can represent different workflow states, or different objects entirely.

## Race cases

### A retry is committed before the informer catches up

1. A completed workflow has `completed=true` and `workflow-archiving-status=Pending`.
2. `argo retry` updates the workflow, removes both labels, and resets its phase to `Unknown`. The UID is unchanged.
3. The controller has not received the watch event yet.
4. The archive worker reads the stale completed and `Pending` copy from the informer.
5. The old snapshot is archived.
6. The merge patch marks the now-running workflow as `Archived`.

A UID check alone would not prevent this because `argo retry` keeps the UID.

### The object changes while the archive write is in progress

After the worker selects the workflow but before the database call returns, the workflow may be:

* retried under the same UID; or
* deleted and recreated with the same namespace and name but a different UID.

When the database call returns, the current merge patch still applies to the live object by name.

Longer database latency increases this window, but a slow database is not required: a stale informer read can enter the same path before the archive call starts.

## Why the existing guards are insufficient

`workflowKeyLock` is local to one controller process. It does not serialize updates from argo-server or other Kubernetes clients.

The checks added in #16577 narrow the delayed-queue cases, but they still read from the informer cache. They cannot prove that the object at the API server still has the UID and labels that were used for the archive decision.

The final condition therefore needs to be enforced by the API server as part of the patch itself.

## Impact

The live workflow can carry `workflow-archiving-status=Archived` while it is running. This can produce misleading API and UI state and affect label selectors or automation that relies on the archiving-status label.

The label may be corrected when the workflow completes again and is marked `Pending`, but that is not guaranteed. The operator only marks it for archiving when archiving is enabled and the workflow matches the archive selector. If either condition is no longer true, the incorrect `Archived` label can remain.

## Proposed fix

Replace the merge patch with a JSON Patch that atomically verifies the object identity and the state that was selected for archiving:

```json
[
{
"op": "test",
"path": "/metadata/uid",
"value": ""
},
{
"op": "test",
"path": "/metadata/resourceVersion",
"value": ""
},
{
"op": "replace",
"path": "/metadata/labels/workflows.argoproj.io~1workflow-archiving-status",
"value": "Archived"
}
]
```

The test is on the version, not on the state. **An earlier draft of this issue proposed testing the UID and the `completed` / `workflow-archiving-status` labels instead, and that is not sufficient — ABA defeats it.** `argo retry` keeps the UID and deletes both labels; when the second run completes, the controller writes them back with the same values. Every value a state test could check has returned, so a patch built from the first run applies cleanly to the second, marks a run that was never archived, and removes the `Pending` label that would otherwise have queued it — so that run is never archived at all.

`resourceVersion` identifies the exact object the archive was taken from, so it subsumes the label tests: if it matches, the labels are the ones the worker read. The UID is kept alongside it because a workflow deleted and recreated under the same name is a different object whose version counter says nothing about this one.

Reading the object immediately before sending the existing merge patch would still leave a race between the read and the write.

A `resourceVersion` precondition does reject a benign unrelated update — an added annotation, say — but rejecting it is the correct outcome: the attempt is retried, and the next one reads the current object and archives that.

### Classifying the failure

A failed `test` comes back as a bare 422 `Invalid` whose message and details say nothing about which operation failed, and the API server maps every other patch application error to 422 as well. So the reason cannot be read off the error, and

```go
if apierrors.IsInvalid(err) { return nil }
```

would swallow a malformed pointer or an admission failure as though the workflow had moved on. On 422, ask the API server what the object looks like now: gone, or a different UID, or no longer completed and `Pending`, is terminal; anything else is returned so the queue retries it and a broken patch keeps failing visibly.

One case deserves care. "The version advanced" must **not** be treated as terminal on its own: the object may have advanced into a state that still needs archiving — a second run that has just completed — and ending the attempt there is how that run's archive would be lost.

## Regression tests

Add deterministic coverage for:

1. a workflow retried with the same UID before the patch is applied;
2. a workflow deleted and recreated under the same name with a different UID;
3. an informer that still contains the completed, `Pending` copy after the API object has already been retried;
4. the normal path, where all tests pass and the label changes to `Archived`;
5. error handling that treats a failed JSON Patch test as terminal while still returning malformed-patch and unrelated validation errors.

In the first three cases, the live object must remain unchanged.

## Related

Found while reviewing #16577. That PR closes the duplicate-archive and retry problems and narrows stale queue entries, but cache-side checks cannot close this API-write race.

This is the same general identity problem addressed in #12636: a namespace/name retained from an earlier observation is not proof that the current object is still the intended target.

I can work on the fix if this approach looks right.

Contributor guide

Open the contributing guide

Research direction

Trace the controller's archiveWorkflowAux entry point and the existing archive worker and patch tests; no file paths are specified in the issue. Reproduce the retry and delete/recreate races, then add deterministic coverage for the normal path and error classification. Done means stale objects remain unchanged while the current object is archived only when the patch conditions hold.

Written by the indexing model from the issue text.

Assessment

Tech stack
go, kubernetes
Domain
backend, distributed-systems, testing-qa
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.