Skip to content

fix(provisioner): close SSH client before reassign, close pipe reader#659

Merged
ArangoGutierrez merged 1 commit intoNVIDIA:mainfrom
ArangoGutierrez:fix/provisioner-ssh-leaks
Feb 13, 2026
Merged

fix(provisioner): close SSH client before reassign, close pipe reader#659
ArangoGutierrez merged 1 commit intoNVIDIA:mainfrom
ArangoGutierrez:fix/provisioner-ssh-leaks

Conversation

@ArangoGutierrez
Copy link
Collaborator

Summary

  • Close existing SSH client before overwriting with new connection in provision()
  • Close io.Pipe reader after WaitGroup completes to prevent goroutine leak

Audit Findings

  • #14 (MEDIUM): SSH client overwritten without closing in provision()
  • Fix keypath handling #29 (LOW): io.Pipe reader never closed

Changes

  • pkg/provisioner/provisioner.go: Add client close guard + pipe reader close

Test plan

  • gofmt — no formatting issues
  • go build — compiles
  • go test ./pkg/... — all tests pass

provision() overwrote p.Client with a new SSH connection without
closing the old one, leaking TCP sockets and goroutines. Also close
the io.Pipe reader after wg.Wait() for completeness.

Audit findings #14 (MEDIUM), NVIDIA#29 (LOW).

Signed-off-by: Carlos Eduardo Arango Gutierrez <eduardoa@nvidia.com>
Copilot AI review requested due to automatic review settings February 13, 2026 07:38
@coveralls
Copy link

Pull Request Test Coverage Report for Build 21978700032

Details

  • 0 of 6 (0.0%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.05%) to 47.456%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/provisioner/provisioner.go 0 6 0.0%
Files with Coverage Reduction New Missed Lines %
pkg/provisioner/provisioner.go 1 2.26%
Totals Coverage Status
Change from base Build 21955389842: -0.05%
Covered Lines: 2500
Relevant Lines: 5268

💛 - Coveralls

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes two resource leak issues in the provisioner identified by an audit: an SSH client that was being overwritten without closing, and an io.Pipe reader that was never closed. The changes prevent resource leaks by properly closing these resources before reassignment and after use.

Changes:

  • Added SSH client close guard in provision() before creating new connection
  • Added io.Pipe reader close after WaitGroup completes


// Close existing client before creating new connection
if p.Client != nil {
_ = p.Client.Close()
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SSH client close logic here differs from the pattern used in resetConnection() and waitForNodeReboot(). Those functions check if the connection is still alive by attempting to create a session before closing. Consider using the same pattern here for consistency:

if p.Client != nil {
    session, err := p.Client.NewSession()
    if err == nil {
        _ = session.Close()
        if err := p.Client.Close(); err != nil {
            return fmt.Errorf("failed to close ssh client: %w", err)
        }
    }
    p.Client = nil
}

This prevents potential issues with closing already-dead connections and follows the established convention in the codebase.

Suggested change
_ = p.Client.Close()
// Try to create a new session to check if connection is alive
session, err := p.Client.NewSession()
if err == nil {
_ = session.Close()
// Connection is alive, close it
if err := p.Client.Close(); err != nil {
return fmt.Errorf("failed to close ssh client: %w", err)
}
}
// If we get here, either the connection was already closed or we couldn't create a session

Copilot uses AI. Check for mistakes.
@ArangoGutierrez ArangoGutierrez merged commit 6e63f12 into NVIDIA:main Feb 13, 2026
25 checks passed
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.

3 participants