Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-60354

Updated pipeline build step does not work with retry, catchError, or warnError

    Details

    • Similar Issues:
    • Released As:
      workflow-basic-steps 2.19, pipeline-build-step 2.11, workflow-step-api 2.22

      Description

      We just upgraded pipeline-build-step from 2.9 to 2.10. The following no longer retries:

      pipeline {
          agent any
      
          stages {
              stage('bug') {
                  steps {
                      retry(3) {
                          echo 'in the retry loop'
                          //throw new Exception('failing in retry loop') // This runs (when uncommented) several times before the pipeline exits with failure.
                          build(job: 'some/job/that/fails', propagate: true) // This only runs once before the pipeline exits with failure.
                      }
                  }
              }
          }
      }
      

      When the sub-build fails, the retry loop exits immediately and does not retry. This didn't used to happen. When raising an error in another way (e.g. the `throw new Exception`, commented out, in the code above), `retry` works as expected.

      A workaround is to try/catch the `build()` call and, in catch, `throw new Exception(...)`. If the caught exception is directly re-thrown, we see the same behavior where `retry` does not loop.

        Attachments

          Issue Links

            Activity

            Hide
            dnusbaum Devin Nusbaum added a comment -

            This is caused by JENKINS-49073, and difficult to fix without breaking JENKINS-44379 (unless we just revert JENKINS-49073).

            The cause is that Pipeline Build Step 2.10 now throws FlowInterruptedException when the build fails instead of AbortException so that it can track the result. retry ignores FlowInterruptedException for reasons discussed in JENKINS-44379 (aborting the build via an input step or the sidebar of the build should bypass retry)

            I'm not really sure how to fix this. I think FlowInterruptedException is overloaded with too many meanings at this point to fix it directly. One approach would be to factor out a ResultCarryingException interface, have FlowInterruptedException and some new FooException implement that interface, and then go through and switch everything that currently just uses FlowInterruptedException for a result to use ResultCarryingException when inspecting exceptions and FooException when creating them, and only use FlowInterruptedException when we only want to capture actual Pipeline interruption.

            Show
            dnusbaum Devin Nusbaum added a comment - This is caused by JENKINS-49073 , and difficult to fix without breaking JENKINS-44379 (unless we just revert JENKINS-49073 ). The cause is that Pipeline Build Step 2.10 now throws FlowInterruptedException when the build fails instead of AbortException so that it can track the result. retry ignores FlowInterruptedException for reasons discussed in JENKINS-44379 (aborting the build via an input step or the sidebar of the build should bypass retry ) I'm not really sure how to fix this. I think FlowInterruptedException is overloaded with too many meanings at this point to fix it directly. One approach would be to factor out a ResultCarryingException interface, have FlowInterruptedException and some new FooException implement that interface, and then go through and switch everything that currently just uses FlowInterruptedException for a result to use ResultCarryingException when inspecting exceptions and FooException when creating them, and only use FlowInterruptedException when we only want to capture actual Pipeline interruption.
            Hide
            dnusbaum Devin Nusbaum added a comment -

            Jesse Glick Noted in this comment that the easiest short-term fix is probably to make retry only rethrow FlowInterruptedException when there is at least one CauseOfInterruption. I did a quick investigation and I think WorkflowRun.doTerm would need to start setting a CauseOfInterruption, but didn't see any other uses that would need to be changed.

            He also noted that another possible fix would be to add a flag to FlowInterrupted exception to indicate actual build interruptions as opposed to otherwise-normal exceptions with a result, and have steps like retry look at that flag to decide whether to rethrow the exception. I think that would be easier than introducing new exception types as in my original idea, and a bit more robust than relying on the presence or absence of a CauseOfInterruption, although we would need to update things like the input step and have them set that flag, so it would require a bit more effort.

            I am also noting that catchError and warnError have the same problem as retry with the new version of build, and will ignore any errors thrown by it by default.

            Show
            dnusbaum Devin Nusbaum added a comment - Jesse Glick Noted in this comment that the easiest short-term fix is probably to make retry only rethrow FlowInterruptedException when there is at least one CauseOfInterruption . I did a quick investigation and I think WorkflowRun.doTerm would need to start setting a CauseOfInterruption , but didn't see any other uses that would need to be changed. He also noted that another possible fix would be to add a flag to FlowInterrupted exception to indicate actual build interruptions as opposed to otherwise-normal exceptions with a result, and have steps like retry look at that flag to decide whether to rethrow the exception. I think that would be easier than introducing new exception types as in my original idea, and a bit more robust than relying on the presence or absence of a CauseOfInterruption , although we would need to update things like the input step and have them set that flag, so it would require a bit more effort. I am also noting that catchError and warnError have the same problem as retry with the new version of build , and will ignore any errors thrown by it by default.
            Hide
            basil Basil Crow added a comment -

            It might be worth considering the use case in JENKINS-51454 as part of the design of this solution.

            Show
            basil Basil Crow added a comment - It might be worth considering the use case in JENKINS-51454 as part of the design of this solution.
            Hide
            dnusbaum Devin Nusbaum added a comment -

            Basil Crow Good point, I commented on that ticket. I think that the approach from this PR could be tweaked a bit to use https://github.com/jenkinsci/workflow-step-api-plugin/pull/51 (if a setter was added) to change the value of FlowInterruptedException.actualInterruption from true to false when the exception propagates past the timeout step that threw the exception, in which case the fix for this issue would fix that issue as well.

            Show
            dnusbaum Devin Nusbaum added a comment - Basil Crow Good point, I commented on that ticket. I think that the approach from this PR could be tweaked a bit to use https://github.com/jenkinsci/workflow-step-api-plugin/pull/51 (if a setter was added) to change the value of FlowInterruptedException.actualInterruption from true to false when the exception propagates past the timeout step that threw the exception, in which case the fix for this issue would fix that issue as well.
            Hide
            dnusbaum Devin Nusbaum added a comment -

            Ok, PRs are up for review:

            catchError and warnError are not as affected as retry as I first thought, because they catch all interruptions by default, and only rethrow them if the user passes catchInterruptions: false (I thought the default was true).

            Show
            dnusbaum Devin Nusbaum added a comment - Ok, PRs are up for review: https://github.com/jenkinsci/workflow-step-api-plugin/pull/51 https://github.com/jenkinsci/pipeline-build-step-plugin/pull/39 https://github.com/jenkinsci/workflow-basic-steps-plugin/pull/102 catchError and warnError are not as affected as retry as I first thought, because they catch all interruptions by default, and only rethrow them if the user passes catchInterruptions: false (I thought the default was true ).
            Hide
            dnusbaum Devin Nusbaum added a comment -

            A fix for this issue has been released in Pipeline: Basic Steps Plugin 2.19 and Pipeline: Build Step Plugin 2.11. Both plugins must be updated for the fix to take effect. Thanks Jonathan B for reporting the issue!

            Show
            dnusbaum Devin Nusbaum added a comment - A fix for this issue has been released in Pipeline: Basic Steps Plugin 2.19 and Pipeline: Build Step Plugin 2.11. Both plugins must be updated for the fix to take effect. Thanks Jonathan B for reporting the issue!

              People

              • Assignee:
                dnusbaum Devin Nusbaum
                Reporter:
                jonathanb1 Jonathan B
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: