[JENKINS-51454] Pipeline retry operation doesn't retry when there is a timeout inside of it

[basil]

See JENKINS-51454. Downstream of jenkinsci/workflow-step-api-plugin#61.

Problem

There are two desired behaviors depending on the placement of timeout and retry relative to each other.

First case (JENKINS-51454):

retry(3) {
  timeout(time: 5, unit: 'MINUTES') {
    // Something that can fail
  }
}

In this case, if the timeout triggers, the desired behavior is for retry to run its body again. This is not the current behavior.

Second case (JENKINS-44379):

timeout(time: 5, unit: 'MINUTES') {
  retry(3) {
    // Something that can fail
  }
}

In this case, if the timeout triggers, the desired behavior is for retry to be aborted without retrying anything. This is the current behavior, which is working as-designed after JENKINS-44379.

Solution

JENKINS-60354 overlaps a bit with this issue. @dwnusbaum suggested that the approach from #81 by @rudolfwalter could be tweaked a bit to use jenkinsci/workflow-step-api-plugin#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 JENKINS-60354 in #102 would fix JENKINS-51454 as well. That is precisely what we are doing in jenkinsci/workflow-step-api-plugin#61 and this PR.

[jglick]

I assume is unlikely to throw exceptions? See special behaviors lost when moving from TailCall. Maybe better to enhance TailCall to make onFailure nonfinal so you can call super but still do some special processing with the exception.

[basil]

Done. I wasn't sure whether to put the call to super.onFailure() before or after the new logic, but I put it before the new logic and the tests still passed.

[jglick]

I am not understanding why all the complexity. Does a simple

Suggested change

	new FlowInterruptedException(Result.ABORTED, exceededTimeout);
	new FlowInterruptedException(Result.ABORTED, false, exceededTimeout);

not solve the issue here, without the upstream PR?

[dwnusbaum]

If you do that, then I think both case 1 and case 2 from the PR description would have the same behavior. (Maybe fine, but IMO that just trades the current frustrations with case 1 to new frustrations with case 2.)

[timja]

@dwnusbaum @car-roll

[basil]

The test that just failed is flaky. I just re-ran it locally and it passed.

[basil]

12:14:18  Cannot contact aci-maven-11-2thrx: hudson.remoting.ChannelClosedException: Channel "hudson.remoting.Channel@68f899af:JNLP4-connect connection from 40.88.240.16/40.88.240.16:6273": Remote call on JNLP4-connect connection from 40.88.240.16/40.88.240.16:6273 failed. The channel is closing down or has closed down

I am stepping away from this for now. I believe the change is ready for merge and release, but the CI build is failing due to problems that I believe are out of my control and not caused by me.

[timja]

Build is green now, cc @dwnusbaum @car-roll @bitwiseman

[car-roll]

@dwnusbaum would you mind taking a look at this since @jglick is not available atm?

[dwnusbaum]

Seems fine, but I added some comments. The main thing I would change would be to call FlowInterruptedException.handle instead of directly printing the causes.

[basil]

All feedback has been addressed, and the build is green. I think it's time to put this bug to rest.

[rajarajeshwarie-kg-18298]

does the timeout be enabled in the retry of the pipeline? I tried the code..when the timeout exceeded, the retry option was working but without timeout.

stage (‘Build’) {

    options {
    retry(2)
    timeout(time: 10, unit: 'MINUTES')
    
    }
    steps {
       //code
    }
    post {  
        success { 
         //code
        }
    }
}

[jglick]

Check in Scripted syntax. If that works, and you only see a problem in Declarative syntax, file a bug report in pipeline-model-definition-plugin.