Skip to content

Conversation

EricHripko
Copy link

This PR proposes a fix for the occasional Process leaked file descriptors issue with this plugin. Please see my comment for the research into this failure and any relevant prior art. It should fix JENKINS-63628.

Testing done

I've added unit tests for this change, and these passed locally ✅

The tests in the current repo are system/integration tests, but (given the nature of the issue) I could not write a system test that reliably reproduces this. This is why this PR adds mockito dependency and a pair of unit tests to verify this functionality.

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests that demonstrate the feature works or the issue is fixed

@EricHripko EricHripko marked this pull request as ready for review June 5, 2025 08:20
@EricHripko
Copy link
Author

Hi @jglick! I see you contributed here recently - would you be able to help with reviewing this plugin? Let me know if I should poke someone else instead/too.

@jglick
Copy link
Member

jglick commented Jun 24, 2025

I do not maintain this plugin; I am not sure if anyone does. My standing advice (as the principal original author) is to not use it.

@jglick
Copy link
Member

jglick commented Jun 24, 2025

Anyway this does not look like the right fix. https://github.com/jenkinsci/jenkins/blob/47d10476078ff6fc938bfd81c2679fde7c4df220/core/src/main/java/hudson/Proc.java#L355-L356 ought to be deleted (a warning in the system log should suffice); but the root problem sounds like something mistaken in this plugin that does indeed leak file handles.

@EricHripko
Copy link
Author

@jglick thank you for clarifying - do you think it's still worth fixing this issue or should I abandon the attempts to do so?

If I understand the plugin code correctly, it intentionally spawns a container/process in the background so that it outlives the Proc invocation:

container = dockerClient.run(env, step.image, step.args, ws, volumes, volumesFromContainers, envReduced, dockerClient.whoAmI(), /* expected to hang until killed */ command);

Follow up pipeline steps seem to be then executed via an equivalent to docker exec in said container.

@jglick
Copy link
Member

jglick commented Jul 7, 2025

I would suggest closing this and deleting the core code I pointed out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants