fix: prevent dfs sorter infinite recursion on cycle. Fixes #13395

[Adrien-D]

Fixes #13395

Motivation

We're encountering an infinite loop because DFS sorter doesn't account for cycles in the graph. If a node is revisited while it’s still being processed, we can enter a cycle

Modifications

To fix this, I track nodes that are currently being visited (i.e., "visiting") in addition to those that are already visited ("discovered") and skip the nodes we are currently visiting.

Verification

Tested on a workflow that reproduces the error, and multiple others to make sure nothing is broken.

Here is the workflow I used

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: stack-overflow-test-
spec:
  podGC:
    strategy: OnWorkflowCompletion
  entrypoint: main
  templates:
    # Main function
    - name: main
      steps:
      - - name: step-a
          template: step-a
          arguments:
            artifacts:
              - name: test
                archive:
                  none: {}
                s3:
                  bucket: my-bucket
                  key: foo/message.txt
                  endpoint: minio.argo.svc.cluster.local:9000
                  insecure: true
                  accessKeySecret:
                    name: my-minio-cred
                    key: accesskey
                  secretKeySecret:
                    name: my-minio-cred
                    key: secretkey
      
      - - name: step-b
          template: step-b
          arguments:
            parameters:
            - name: test
              value: "{{=sprig.fromJson(steps['step-a'].outputs.parameters['test'])['this']}}"
          

      - - name: step-c
          when: "{{=steps['step-b'].outputs.parameters['test'] != ''}}"
          template: step-c
          arguments:
            parameters:
            - name: test
              value: "{{=sprig.fromJson(steps['step-b'].outputs.parameters['test'])['that']}}"
          

      - - name: step-d
          when: "{{=steps['step-b'].outputs.parameters['test'] != ''}}"
          arguments:
            artifacts:
            - name: test
              from: '{{steps.step-b.outputs.artifacts.test}}' 
          inline: 
            inputs:
              artifacts:
              - name: test
                path: /tmp/test
                archive:
                  none: {}
            outputs:
              artifacts:
              - name: test
                path: /tmp/test
                archive:
                  none: {}
                s3:
                  bucket: my-bucket
                  key: foo/message.txt
                  endpoint: minio.argo.svc.cluster.local:9000
                  insecure: true
                  accessKeySecret:
                    name: my-minio-cred
                    key: accesskey
                  secretKeySecret:
                    name: my-minio-cred
                    key: secretkey
            container:
              image: docker.io/argoproj/argosay:v2
              command: [/argosay, echo, 'This is a test']

    - name: step-a
      inputs:
        artifacts:
        - name: test
          path: /tmp/test
          archive:
            none: {}
      outputs:
        artifacts:
        - name: test
          path: /tmp/test
          archive:
            none: {}
        parameters:
        - name: test
          valueFrom:
            path: /tmp/test
      container:
        image: docker.io/argoproj/argosay:v2
        command: [/argosay, echo, 'This is a test']
    - name: step-b
      inputs:
        parameters:
        - name: test
      outputs:
        artifacts:
        - name: test
          path: /tmp/test
          archive:
            none: {}
        parameters:
        - name: test
          valueFrom:
            path: /tmp/test
      script:
        image: docker.io/library/alpine:3
        command: [/bin/sh, -xec]
        env:
        - name: TEST
          value: '{{inputs.parameters.test}}'
        args:
        - |
          echo '{"that": "'"${TEST}"'"}' > /tmp/test
    - name: step-c
      inputs:
        parameters:
        - name: test
      
      container:
        image: docker.io/argoproj/argosay:v2
        command: [/argosay, echo, 'This is a test']

Normal render	Fast render

Documentation

[JPZ13]

@Adrien-D - your Javascript/Typescript knowledge is more current than mine. Do you know how a Set's has function does object comparison? I saw some stuff that said it was a reference comparison and others saying it's a deep equals comparison. In both cases, would a node ID be better?

For the reference comparison, if an equivalent node gets compared but has a different reference due to react deep copying it on the next render, wouldn't has return a false negative?

Similarly for the deep equality, if the node gets a field added or modified, wouldn't has also produce a false negative as well?

I'll default to your expertise, but flagging in case these scenarios are valid

[Adrien-D]

@JPZ13 Great catch, you're absolutely right. Set.has() is reference-based, example here. So that could definitely lead to a false comparison, but not in this case here because Node is actually the node ID (naming is confusing I agree). So it's a string that shouldn't lead to this issue.

[JPZ13]

What's in a name? 😁

Thanks, @Adrien-D - approving now

[JPZ13]

What's in a name? 😁

Thanks, @Adrien-D - approving now

[terrytangyuan]

Thanks for the fix!

[Joibel]

/cherry-pick release-3.6