fix: prevent IdleTimeout from closing control connection during activ…

client_handler.go

@@ -455,9 +455,9 @@ func (c *clientHandler) readCommand() bool {
 	}
 
 	if err != nil {
-		c.handleCommandsStreamError(err)
+		shouldDisconnect := c.handleCommandsStreamError(err)
 
-		return true
+		return shouldDisconnect
 	}
 
 	line := string(lineSlice)
@@ -471,11 +471,31 @@ func (c *clientHandler) readCommand() bool {
 	return false
 }
 
-func (c *clientHandler) handleCommandsStreamError(err error) {
+func (c *clientHandler) handleCommandsStreamError(err error) bool {
 	// florent(2018-01-14): #58: IDLE timeout: Adding some code to deal with the deadline
 	var errNetError net.Error
 	if errors.As(err, &errNetError) { //nolint:nestif // too much effort to change for now
 		if errNetError.Timeout() {
+			// Check if there's an active data transfer before closing the control connection
+			c.transferMu.Lock()
+			hasActiveTransfer := c.isTransferOpen
+			c.transferMu.Unlock()
+
+			if hasActiveTransfer {
+				// If there's an active data transfer, extend the deadline and continue
+				extendedDeadline := time.Now().Add(time.Duration(time.Second.Nanoseconds() * int64(c.server.settings.IdleTimeout)))
+				if errSet := c.conn.SetDeadline(extendedDeadline); errSet != nil {
+					c.logger.Error("Could not extend read deadline during active transfer", "err", errSet)
+				}
+
+				if c.debug {
+					c.logger.Debug("Idle timeout occurred during active transfer, extending deadline")
+				}
+
+				// Don't disconnect - the transfer is still active
+				return false
+			}
+
 			// We have to extend the deadline now
 			if errSet := c.conn.SetDeadline(time.Now().Add(time.Minute)); errSet != nil {
 				c.logger.Error("Could not set read deadline", "err", errSet)
@@ -490,19 +510,23 @@ func (c *clientHandler) handleCommandsStreamError(err error) {
 				c.logger.Error("Flush error", "err", errFlush)
 			}
 
-			return
+			return true
 		}
 
 		c.logger.Error("Network error", "err", err)
-	} else {
-		if errors.Is(err, io.EOF) {
-			if c.debug {
-				c.logger.Debug("Client disconnected", "clean", false)
-			}
-		} else {
-			c.logger.Error("Read error", "err", err)
+
+		return true
+	}
+
+	if errors.Is(err, io.EOF) {
+		if c.debug {
+			c.logger.Debug("Client disconnected", "clean", false)
 		}
+	} else {
+		c.logger.Error("Read error", "err", err)
 	}
+
+	return true
 }
 
 // handleCommand takes care of executing the received line

idle_timeout_transfer_test.go

@@ -0,0 +1,66 @@
+package ftpserver
+
+import (
+	"bytes"
+	"testing"
+	"time"
+
+	"github.com/secsy/goftp"
+	"github.com/stretchr/testify/require"
+)
+
+// TestIdleTimeoutDuringTransfer verifies that the idle timeout doesn't close
+// the control connection when a data transfer is active.
+func TestIdleTimeoutDuringTransfer(t *testing.T) {
+	// Create a server with a very short idle timeout
+	// The test driver adds 500ms delay for files with "delay-io" in the name
+	server := NewTestServerWithTestDriver(t, &TestServerDriver{
+		Debug: true,
+		Settings: &Settings{
+			IdleTimeout: 1, // 1 second idle timeout
+		},
+	})
+
+	conf := goftp.Config{
+		User:     authUser,
+		Password: authPass,
+	}
+
+	client, err := goftp.DialConfig(conf, server.Addr())
+	require.NoError(t, err, "Couldn't connect")
+
+	defer func() { panicOnError(client.Close()) }()
+
+	// Create test data - size chosen so that with 500ms delays per read,
+	// the transfer will take longer than the 1 second idle timeout
+	data := make([]byte, 1024*1024) // 1MB
+	for i := range data {
+		data[i] = byte(i % 256)
+	}
+
+	// Upload the file with "delay-io" in the name to trigger slow I/O
+	// This will cause each Read() operation to take 500ms
+	err = client.Store("delay-io-test.bin", bytes.NewReader(data))
+	require.NoError(t, err, "Failed to upload file")
+
+	// Download the file - this will trigger multiple 500ms delays
+	// Total time will exceed the 1 second idle timeout
+	// The server should extend the deadline during the active transfer
+	buf := &bytes.Buffer{}
+	start := time.Now()
+	err = client.Retrieve("delay-io-test.bin", buf)
+	elapsed := time.Since(start)
+
+	require.NoError(t, err, "Transfer should succeed despite idle timeout")
+	require.Equal(t, len(data), buf.Len(), "Downloaded data should match uploaded data")
+	require.Equal(t, data, buf.Bytes(), "Downloaded content should match uploaded content")
+
+	// Verify the transfer took longer than the idle timeout
+	// This proves the deadline was extended during the transfer
+	require.Greater(t, elapsed, time.Duration(server.settings.IdleTimeout)*time.Second,
+		"Transfer should take longer than idle timeout to verify deadline extension worked")
+
+	// Verify the connection is still alive after the transfer
+	_, err = client.ReadDir("/")
+	require.NoError(t, err, "Connection should still be alive after long transfer")
+}

transfer_test.go

@@ -1360,3 +1360,66 @@ func getPortFromEPSVResponse(t *testing.T, resp string) int {
 
 	return port
 }
+
+// TestConnectionCloseDuringTransfer tests the behavior when the control connection is closed
+// during an active data transfer in passive mode. This ensures proper cleanup and error handling.
+func TestConnectionCloseDuringTransfer(t *testing.T) {
+	t.Parallel()
+
+	server := NewTestServer(t, false)
+
+	conf := goftp.Config{
+		User:     authUser,
+		Password: authPass,
+	}
+
+	client, err := goftp.DialConfig(conf, server.Addr())
+	require.NoError(t, err, "Couldn't connect")
+
+	// Upload a file first so we have something to download
+	file := createTemporaryFile(t, 10*1024*1024) // 10MB file
+	err = client.Store("large-file.bin", file)
+	require.NoError(t, err, "Failed to upload file")
+
+	// Open a raw connection to have more control
+	raw, err := client.OpenRawConn()
+	require.NoError(t, err)
+
+	// Prepare data connection
+	_, err = raw.PrepareDataConn()
+	require.NoError(t, err)
+
+	// Start the RETR command
+	returnCode, response, err := raw.SendCommand("RETR large-file.bin")
+	require.NoError(t, err)
+	require.Equal(t, StatusFileStatusOK, returnCode, response)
+
+	// Give the transfer a moment to start
+	time.Sleep(50 * time.Millisecond)
+
+	// Now close the raw connection abruptly during the transfer
+	// This simulates a network disconnection or client crash
+	err = raw.Close()
+	require.NoError(t, err)
+
+	// Close the main client connection as well
+	_ = client.Close()
+	// We expect an error here since the connection is already closed/broken
+	// but we don't want to fail the test - this is expected behavior
+
+	// Give the server time to clean up
+	time.Sleep(100 * time.Millisecond)
+
+	// Verify we can establish a new connection and the server is still functional
+	newClient, err := goftp.DialConfig(conf, server.Addr())
+	require.NoError(t, err, "Server should still be functional after connection close during transfer")
+
+	defer func() {
+		err = newClient.Close()
+		require.NoError(t, err)
+	}()
+
+	// Verify the file is still there and accessible
+	_, err = newClient.Stat("large-file.bin")
+	require.NoError(t, err, "File should still be accessible after interrupted transfer")
+}