diff --git a/backend/sftp/ssh_external.go b/backend/sftp/ssh_external.go index a42cdd7cc..caa472bb6 100644 --- a/backend/sftp/ssh_external.go +++ b/backend/sftp/ssh_external.go @@ -10,6 +10,7 @@ import ( "os/exec" "slices" "strings" + "sync" "time" "github.com/rclone/rclone/fs" @@ -50,6 +51,9 @@ func (s *sshClientExternal) Close() error { func (s *sshClientExternal) NewSession() (sshSession, error) { session := s.f.newSSHSessionExternal() if s.session == nil { + // Store the first session so Wait() and Close() can use it + s.session = session + } else { fs.Debugf(s.f, "ssh external: creating additional session") } return session, nil @@ -76,6 +80,8 @@ type sshSessionExternal struct { cancel func() startCalled bool runningSFTP bool + waitOnce sync.Once // ensure Wait() is only called once + waitErr error // result of the Wait() call } func (f *Fs) newSSHSessionExternal() *sshSessionExternal { @@ -175,16 +181,17 @@ func (s *sshSessionExternal) exited() bool { // Wait for the command to exit func (s *sshSessionExternal) Wait() error { - if s.exited() { - return nil - } - err := s.cmd.Wait() - if err == nil { - fs.Debugf(s.f, "ssh external: command exited OK") - } else { - fs.Debugf(s.f, "ssh external: command exited with error: %v", err) - } - return err + // Use sync.Once to ensure we only wait for the process once. + // This is safe even if Wait() is called from multiple goroutines. + s.waitOnce.Do(func() { + s.waitErr = s.cmd.Wait() + if s.waitErr == nil { + fs.Debugf(s.f, "ssh external: command exited OK") + } else { + fs.Debugf(s.f, "ssh external: command exited with error: %v", s.waitErr) + } + }) + return s.waitErr } // Run runs cmd on the remote host. Typically, the remote diff --git a/backend/sftp/ssh_external_test.go b/backend/sftp/ssh_external_test.go new file mode 100644 index 000000000..b2a51f904 --- /dev/null +++ b/backend/sftp/ssh_external_test.go @@ -0,0 +1,84 @@ +//go:build !plan9 + +package sftp + +import ( + "testing" + "time" + + "github.com/rclone/rclone/fs" + "github.com/stretchr/testify/assert" +) + +// TestSSHExternalWaitMultipleCalls verifies that calling Wait() multiple times +// doesn't cause zombie processes +func TestSSHExternalWaitMultipleCalls(t *testing.T) { + // Create a minimal Fs object for testing + opt := &Options{ + SSH: fs.SpaceSepList{"echo", "test"}, + } + + f := &Fs{ + opt: *opt, + } + + // Create a new SSH session + session := f.newSSHSessionExternal() + + // Start a simple command that exits quickly + err := session.Start("exit 0") + assert.NoError(t, err) + + // Give the command time to complete + time.Sleep(100 * time.Millisecond) + + // Call Wait() multiple times - this should not cause issues + err1 := session.Wait() + err2 := session.Wait() + err3 := session.Wait() + + // All calls should return the same result (no error in this case) + assert.NoError(t, err1) + assert.NoError(t, err2) + assert.NoError(t, err3) + + // Verify the process has exited + assert.True(t, session.exited()) +} + +// TestSSHExternalCloseMultipleCalls verifies that calling Close() multiple times +// followed by Wait() calls doesn't cause zombie processes +func TestSSHExternalCloseMultipleCalls(t *testing.T) { + // Create a minimal Fs object for testing + opt := &Options{ + SSH: fs.SpaceSepList{"sleep", "10"}, + } + + f := &Fs{ + opt: *opt, + } + + // Create a new SSH session + session := f.newSSHSessionExternal() + + // Start a long-running command + err := session.Start("sleep 10") + if err != nil { + t.Skip("Cannot start sleep command:", err) + } + + // Close should cancel and wait for the process + _ = session.Close() + + // Additional Wait() calls should return the same error + err2 := session.Wait() + err3 := session.Wait() + + // All should complete without panicking + // err1 could be nil or an error depending on how the process was killed + // err2 and err3 should be the same + assert.Equal(t, err2, err3, "Subsequent Wait() calls should return same result") + + // Verify the process has exited + assert.True(t, session.exited()) +}