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

Revisit use of $JENKINS_SERVER_COOKIE and Launcher.kill

      PlaceholderExecutable needs an environment variable to add to the context to ensure that Launcher.kill, called when the block exits, will clean up any stray external processes run on this node.

      Originally the name of this variable was chosen to be JENKINS_SERVER_COOKIE, which is set by Job (to a confidential hex key) and normally used by AbstractBuild for this purpose. Unlike in that case, for Workflow the value is overridden (to a random UUID) for each node block, since there may be several. I think the intent was to use the same variable name to suppress any unwanted kills coming from Jenkins core code, but there seem to be none. Anyway after the many refactorings of environment variable handling in Workflow, it turns out this is dead code!

      • FileMonitoringTask sets its own value for JENKINS_SERVER_COOKIE, to durable-<workspaceHash>, which overrides any other value, and is used by stop. So within a shell step, $JENKINS_SERVER_COOKIE is the one from durable-task, not PlaceholderExecutable.
      • env.JENKINS_SERVER_COOKIE picks up the value from Job, due to the precedence order in getEffectiveEnvironment.

      So it seems that while the intention is still good (we would like a final check that any processes started inside node are killed when the block exits), the implementation is not right. Probably PlaceholderExecutable just needs to pick an unrelated environment variable for this purpose. And this logic needs to be tested; for example:

      def file = new File(...);
      node {
        sh "(sleep 5; touch $file) &"
      }
      sleep 10
      assert !file.exists()
      

          [JENKINS-28182] Revisit use of $JENKINS_SERVER_COOKIE and Launcher.kill

          Jesse Glick created issue -

          Jesse Glick added a comment -

          JENKINS-28131 also discusses passing some other environment variables from PlaceholderExecutable, so would be easy to do at the same time.

          Jesse Glick added a comment - JENKINS-28131 also discusses passing some other environment variables from PlaceholderExecutable , so would be easy to do at the same time.
          Jesse Glick made changes -
          Link New: This issue is related to JENKINS-28131 [ JENKINS-28131 ]

          Jesse Glick added a comment -

          The use of JENKINS-25938 in the lts-609 branch also moves the Launcher.kill to the finish method (where it belonged to begin with), and moves the definition of COOKIE_VAR, so there could be merge conflicts if done before that branch is merged.

          Jesse Glick added a comment - The use of JENKINS-25938 in the lts-609 branch also moves the Launcher.kill to the finish method (where it belonged to begin with), and moves the definition of COOKIE_VAR , so there could be merge conflicts if done before that branch is merged.
          Jesse Glick made changes -
          Link New: This issue depends on JENKINS-25938 [ JENKINS-25938 ]
          Jesse Glick made changes -
          Epic Link New: JENKINS-35399 [ 171192 ]
          R. Tyler Croy made changes -
          Workflow Original: JNJira [ 162900 ] New: JNJira + In-Review [ 181065 ]
          Andrew Bayer made changes -
          Component/s New: pipeline-general [ 21692 ]
          Andrew Bayer made changes -
          Component/s Original: workflow-plugin [ 18820 ]
          Jesse Glick made changes -
          Component/s New: workflow-durable-task-step-plugin [ 21715 ]
          Component/s Original: pipeline [ 21692 ]

            mmitche Matthew Mitchell
            jglick Jesse Glick
            Votes:
            1 Vote for this issue
            Watchers:
            9 Start watching this issue

              Created:
              Updated:
              Resolved: