unlock unlimited code runs for $79, limited time →
NOSSLETTER.techLIVE
back to OSSpedia
argoproj/argo-cd
mid8/10
error-handlingconsistency-modelstesting-strategy

fix(sync): correctly set operationState values on retry (#26530) (cherry-pick #28778 for 3.5)

Project: argoproj/argo-cd | Language: Go | Concepts: error-handling, consistency-models, testing-strategy | Difficulty: mid
Link: https://github.com/argoproj/argo-cd/pull/29431

The problem

When an Argo CD Application's sync is retried, the controller could leak stale or incorrect nested values in the /status/operationState field. For example, fields like the sync revision or hook phase could incorrectly persist values from a previous operation, leading to confusing or incorrect status reporting. This happened because the controller used a merge-patch to update only parts of the operation state, which didn't reliably clear out fields that should be removed.

Why it's non-trivial

A naive fix might just update the fields that changed, but this approach misses cases where fields should be deleted (e.g., when a retry omits a field that was present before). Merge-patch semantics don't remove fields that are omitted in the patch—they only overwrite or add. This means old values can "leak" into the new state unless the controller explicitly nulls them out, which is error-prone and doesn't scale as the schema evolves.

The approach

Switch from merge-patch to JSON Patch to atomically replace the operationState subtree:

-	patch := map[string]any{
-		"status": map[string]any{
-			"operationState": state,
-		},
-	}
-	if state.Phase.Completed() {
-		// If operation is completed, clear the operation field to indicate no operation is
-		// in progress.
-		patch["operation"] = nil
-	}
...
-	patchJSON, err := json.Marshal(patch)
+	// Replace the whole operationState since we re-evaluated the whole object
+	patchOps := []map[string]any{
+		{"op": "add", "path": "/status/operationState", "value": state},
+	}
+	if state.Phase.Completed() {
+		// If operation is completed, clear the operation field to indicate no operation is in progress.
+		patchOps = append(patchOps, map[string]any{"op": "add", "path": "/operation", "value": nil})
+	}
+	patchJSON, err := json.Marshal(patchOps)

This change constructs a JSON Patch operation that atomically replaces the entire /status/operationState subtree, ensuring no stale nested fields persist. If the operation is completed, it also clears the top-level /operation field in the same patch.

Change the patch type from MergePatch to JSONPatch:

-		_, err := ctrl.PatchAppWithWriteBack(context.Background(), app.Name, app.Namespace, types.MergePatchType, patchJSON, metav1.PatchOptions{})
+		_, err := ctrl.PatchAppWithWriteBack(context.Background(), app.Name, app.Namespace, types.JSONPatchType, patchJSON, metav1.PatchOptions{})

This ensures the patch is interpreted as a JSON Patch, not a merge patch, so the replace semantics are honored.

Update tests to assert on persisted Application state, not just outgoing patch payloads:

-	phase, _, _ := unstructured.NestedString(receivedPatch, "status", "operationState", "phase")
-	message, _, _ := unstructured.NestedString(receivedPatch, "status", "operationState", "message")
-	assert.Equal(t, string(synccommon.OperationError), phase)
-	assert.Equal(t, "Failed to load application project: error getting app project \"default\": appproject.argoproj.io \"default\" not found", message)
+	patchedApp, err := ctrl.applicationClientset.ArgoprojV1alpha1().Applications(app.Namespace).Get(t.Context(), app.Name, metav1.GetOptions{})
+	require.NoError(t, err)
+	require.NotNil(t, patchedApp.Status.OperationState)
+	assert.Equal(t, synccommon.OperationError, patchedApp.Status.OperationState.Phase)
+	assert.Equal(t, "Failed to load application project: error getting app project \"default\": appproject.argoproj.io \"default\" not found", patchedApp.Status.OperationState.Message)

Tests now verify the actual state stored in the fake client, ensuring that the full subtree replacement works as intended and no stale data remains.

Add a regression test to ensure empty fields clear previous values:

func TestSetOperationState_EmptySyncRevisionClearsPreviousRevision(t *testing.T) {
	app := newFakeApp()
	require.Equal(t, "HEAD", app.Status.OperationState.Operation.Sync.Revision)
	...
	ctrl.setOperationState(app, &v1alpha1.OperationState{
		Phase:     synccommon.OperationRunning,
		StartedAt: metav1.Now(),
		Operation: v1alpha1.Operation{
			Sync:  &v1alpha1.SyncOperation{},
			Retry: v1alpha1.RetryStrategy{Limit: 5},
		},
	})
	...
	assert.Empty(t, patchedApp.Status.OperationState.Operation.Sync.Revision,
		"empty Sync.Revision (omitempty) must clear the previous revision if empty instead of preserving it")
}

This test proves that when a field (like Sync.Revision) is omitted in a retry, it is actually removed from the stored state, not left over from a previous operation.

From the review thread

No notable review discussion on this PR.

Takeaways

  • Use atomic subtree replacement when updating nested status to prevent stale data leaks.
    When a controller re-evaluates an entire nested object, replace the whole subtree instead of merging, to ensure omitted fields are cleared.

  • Choose the right patch type for your update semantics.
    Use JSON Patch for explicit add/replace/remove operations, not MergePatch, when you need to remove fields or ensure strict consistency.

  • Tests should verify persisted state, not just outgoing requests.
    Unit tests that only check the patch payload can miss bugs where the API server's merge semantics don't match expectations; always assert on the stored resource state.

  • Beware of missing parent objects in patch paths.
    When using JSON Patch to add a nested field, ensure the parent object exists or handle its creation, especially if the parent is optional in the schema.