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

ssh-agent-plugin leaking some ssh-agent processes

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Closed (View Workflow)
    • Priority: Major
    • Resolution: Fixed
    • Component/s: core, ssh-agent-plugin
    • Labels:
      None
    • Environment:
      Jenkins 2.32.3, 2.190.2
      ssh-agent-plugin 1.15, 1.17
    • Similar Issues:
    • Released As:
      Jenkins 2.257

      Description

      When a job with the SSHAgentBuildWrapper enabled fails very early (for instance during SCM checkout), an ssh-agent process is left behind. The issue is that the SSHAgentEnvironment is instantiated very early (from preCheckout), but its tearDown method will only be called if execution reaches BuildExecution.doRun (which comes after the SCM checkout phase in AbstractBuildExecution.run).

      Before ssh-agent-plugin 1.14, there was no ssh-agent process, so the issue with some SSHAgentEnvironment not being torn down was less visible (but probably there was already some other kind of less obvious resources leaks with AgentServer not being properly closed).

      This kind of issue with some Environment not being properly torn down can happen as soon as they are not instantiated from BuildWrapper.setUp, but from earlier phases (like BuildWrapper.preCheckout or RunListener.setUpEnvironment). As such, maybe that's something that should be fixed in core (maybe in AbstractBuildExecution.run) rather than specifically in the ssh-agent-plugin, I don't know...

      I've written and attached a "generic workaround" RunListener, which tries to detect this situation from onComplete, and call tearDown for all Environment if it has not been done already. It's not something I propose for inclusion, but rather some code to exhibit the issue. If an ssh-agent specific fix is desirable, then a similar approach might be an option (but targeting SSHAgentEnvironment only).

        Attachments

          Activity

          Hide
          tom_gl Thomas de Grenier de Latour added a comment -

          Revisiting this old issue again, I realize that SimpleBuildWrapper exposes a variation of what's done in the SSHAgentBuildWrapper, allowing to setup an Environment before checkout: SimpleBuildWrapper#runPreCheckout

          If such a SimpleWrapperWrapper provides a Disposer and is run as part of an AbstractBuild which fails early (in SCM checkout), then the disposer won't be called.

          This flaw is again not documented (just like Environment#tearDown says nothing about not always being called on build failure), and makes me lean toward the "it's a core bug" interpretation of this issue.

          I will submit a PR with a test case (using SimpleBuildWrapper), and a possible fix.

          Show
          tom_gl Thomas de Grenier de Latour added a comment - Revisiting this old issue again, I realize that SimpleBuildWrapper exposes a variation of what's done in the SSHAgentBuildWrapper , allowing to setup an Environment before checkout: SimpleBuildWrapper#runPreCheckout If such a SimpleWrapperWrapper provides a Disposer and is run as part of an AbstractBuild which fails early (in SCM checkout), then the disposer won't be called. This flaw is again not documented (just like Environment#tearDown says nothing about not always being called on build failure), and makes me lean toward the " it's a core bug " interpretation of this issue. I will submit a PR with a test case (using SimpleBuildWrapper ), and a possible fix.
          Show
          tom_gl Thomas de Grenier de Latour added a comment - PR submitted: https://github.com/jenkinsci/jenkins/pull/4517
          Hide
          oleg_nenashev Oleg Nenashev added a comment -

          This ticket was mis-categorized in https://www.jenkins.io/changelog/#v2.257 as a Developer fix while the issue seems to be a real defect. It looks like a real defect which would benefit from backporting

          Show
          oleg_nenashev Oleg Nenashev added a comment - This ticket was mis-categorized in https://www.jenkins.io/changelog/#v2.257  as a Developer fix while the issue seems to be a real defect. It looks like a real defect which would benefit from backporting
          Hide
          oleg_nenashev Oleg Nenashev added a comment - - edited

          Backporting note: There is new API introduced. Maybe it makes sense to mark API as restricted while backporting

          rfe: Developer: new static utility method Result#combine(Result,Result) to get the worst of two (nullable) build results

          Show
          oleg_nenashev Oleg Nenashev added a comment - - edited Backporting note: There is new API introduced. Maybe it makes sense to mark API as restricted while backporting rfe: Developer: new static utility method  Result#combine(Result,Result)  to get the worst of two (nullable) build results
          Hide
          olivergondza Oliver Gondža added a comment -

          Not relevant given the choice of LTS baseline: 2.263

          Show
          olivergondza Oliver Gondža added a comment - Not relevant given the choice of LTS baseline: 2.263

            People

            Assignee:
            tom_gl Thomas de Grenier de Latour
            Reporter:
            tom_gl Thomas de Grenier de Latour
            Votes:
            4 Vote for this issue
            Watchers:
            5 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved: