fix: handle closed connection errors gracefully (#602)

* fix: handle closed connection errors gracefully

When a client disconnects immediately after connecting (before sending
any commands), the server would log multiple error messages:
- "Network error" from SetDeadline
- "Network error" or "Read error" from ReadLine

This change detects "use of closed network connection" and "connection
reset by peer" errors and handles them as normal disconnects instead
of errors. This reduces log noise and provides clearer debugging info.

The fix addresses two code paths:
1. In readCommand(): Check if SetDeadline fails due to closed connection
   and return early instead of trying to read
2. In handleCommandsStreamError(): Treat closed connection errors as
   normal disconnects (debug level) instead of errors

Fixes #710

* test: add tests for graceful closed connection handling

Add unit and integration tests to increase coverage for the closed
connection error handling introduced in the previous commit:

- TestHandleCommandsStreamError: Unit tests covering various error types
  including io.EOF, non-net.Error closed connections, net.Error closed
  connections, and generic errors. Improves handleCommandsStreamError
  coverage from 71.4% to 91.4%.

- TestImmediateClientDisconnect: Integration test verifying the server
  handles clients that connect and immediately disconnect (common for
  FTP clients that probe connections).

- TestMultipleImmediateDisconnects: Stress test simulating rapid
  connect/disconnect cycles to verify server stability.

Overall test coverage improved from 91.7% to 92.1%.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Author: fclairamb

client_handler.go

@@ -451,6 +451,15 @@ func (c *clientHandler) readCommand() bool {
 	if c.server.settings.IdleTimeout > 0 {
 		if err := c.conn.SetDeadline(
 			time.Now().Add(time.Duration(time.Second.Nanoseconds() * int64(c.server.settings.IdleTimeout)))); err != nil {
+			// If the connection is already closed, return early instead of trying to read
+			if isClosedConnError(err) {
+				if c.debug {
+					c.logger.Debug("Client disconnected before first command")
+				}
+
+				return true
+			}
+
 			c.logger.Error("Network error", "err", err)
 		}
 	}
@@ -525,11 +534,29 @@ func (c *clientHandler) handleCommandsStreamError(err error) bool {
 			return true
 		}
 
+		// Check if this is a closed connection error - treat as normal disconnect
+		if isClosedConnError(err) {
+			if c.debug {
+				c.logger.Debug("Client disconnected", "clean", false)
+			}
+
+			return true
+		}
+
 		c.logger.Error("Network error", "err", err)
 
 		return true
 	}
 
+	// Check for closed connection errors before logging as error
+	if isClosedConnError(err) {
+		if c.debug {
+			c.logger.Debug("Client disconnected", "clean", false)
+		}
+
+		return true
+	}
+
 	if errors.Is(err, io.EOF) {
 		if c.debug {
 			c.logger.Debug("Client disconnected", "clean", false)

client_handler_test.go

@@ -1,8 +1,12 @@
 package ftpserver
 
 import (
+	"bufio"
+	"bytes"
 	"errors"
 	"fmt"
+	"io"
+	"log/slog"
 	"net"
 	"slices"
 	"sync"
@@ -493,10 +497,12 @@ func TestExtraData(t *testing.T) {
 }
 
 var (
-	errClosedConn    = net.ErrClosed
-	errConnReset     = errors.New("connection reset by peer")
-	errOther         = errors.New("some other error")
-	errWrappedClosed = fmt.Errorf("failed: %w", errClosedConn)
+	errClosedConn     = net.ErrClosed
+	errConnReset      = errors.New("connection reset by peer")
+	errOther          = errors.New("some other error")
+	errWrappedClosed  = fmt.Errorf("failed: %w", errClosedConn)
+	errClosedConnText = errors.New("use of closed network connection")
+	errRandomError    = errors.New("some random error")
 )
 
 func TestIsClosedConnError(t *testing.T) {
@@ -523,6 +529,72 @@ func TestIsClosedConnError(t *testing.T) {
 	}
 }
 
+// mockNetError implements net.Error for testing
+type mockNetError struct {
+	msg       string
+	timeout   bool
+	temporary bool
+}
+
+func (e *mockNetError) Error() string   { return e.msg }
+func (e *mockNetError) Timeout() bool   { return e.timeout }
+func (e *mockNetError) Temporary() bool { return e.temporary }
+
+// testHandleCommandsStreamErrorCase represents a test case for handleCommandsStreamError.
+type testHandleCommandsStreamErrorCase struct {
+	name               string
+	err                error
+	debug              bool
+	expectedDisconnect bool
+}
+
+// getHandleCommandsStreamErrorTestCases returns all test cases for handleCommandsStreamError.
+func getHandleCommandsStreamErrorTestCases() []testHandleCommandsStreamErrorCase {
+	return []testHandleCommandsStreamErrorCase{
+		{name: "EOF with debug", err: io.EOF, debug: true, expectedDisconnect: true},
+		{name: "EOF without debug", err: io.EOF, debug: false, expectedDisconnect: true},
+		{name: "non-net.Error closed with debug", err: errClosedConnText, debug: true, expectedDisconnect: true},
+		{name: "non-net.Error closed without debug", err: errClosedConnText, debug: false, expectedDisconnect: true},
+		{name: "non-net.Error reset", err: errConnReset, debug: true, expectedDisconnect: true},
+		{name: "net.Error closed with debug", err: &mockNetError{msg: "use of closed network connection"},
+			debug: true, expectedDisconnect: true},
+		{name: "net.Error closed without debug", err: &mockNetError{msg: "use of closed network connection"},
+			debug: false, expectedDisconnect: true},
+		{name: "net.Error other", err: &mockNetError{msg: "other network error"}, debug: false, expectedDisconnect: true},
+		{name: "generic error", err: errRandomError, debug: false, expectedDisconnect: true},
+	}
+}
+
+// TestHandleCommandsStreamError tests the handleCommandsStreamError function
+// with various error types to ensure proper handling of closed connections.
+func TestHandleCommandsStreamError(t *testing.T) {
+	t.Parallel()
+
+	//nolint:sloglint // DiscardHandler requires Go 1.23+
+	logger := slog.New(slog.NewTextHandler(io.Discard, nil))
+
+	for _, testCase := range getHandleCommandsStreamErrorTestCases() {
+		t.Run(testCase.name, func(t *testing.T) {
+			t.Parallel()
+
+			buf := bytes.Buffer{}
+			server := &FtpServer{
+				settings: &Settings{IdleTimeout: 0},
+				Logger:   logger,
+			}
+			handler := &clientHandler{
+				writer: bufio.NewWriter(&buf),
+				server: server,
+				logger: logger,
+				debug:  testCase.debug,
+			}
+
+			result := handler.handleCommandsStreamError(testCase.err)
+			assert.Equal(t, testCase.expectedDisconnect, result)
+		})
+	}
+}
+
 func TestDeflateReadWriterFlush(t *testing.T) {
 	t.Parallel()
 
@@ -575,3 +647,92 @@ func (m *mockReadWriter) Read(_ []byte) (int, error) {
 
 	return 0, nil
 }
+
+// TestImmediateClientDisconnect tests that when a client connects and immediately
+// disconnects (before sending any FTP commands), the server handles it gracefully
+// without logging errors. This is common behavior for FTP clients that probe
+// connections or do quick connectivity checks.
+func TestImmediateClientDisconnect(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name  string
+		debug bool
+	}{
+		{name: "with debug", debug: true},
+		{name: "without debug", debug: false},
+	}
+
+	for _, testCase := range tests {
+		t.Run(testCase.name, func(t *testing.T) {
+			t.Parallel()
+
+			server := NewTestServer(t, testCase.debug)
+			dialer := &net.Dialer{Timeout: 5 * time.Second}
+
+			// Connect and immediately close without sending any commands
+			conn, err := dialer.DialContext(t.Context(), "tcp", server.Addr())
+			require.NoError(t, err)
+
+			// Read the welcome message to ensure the server has started handling us
+			buf := make([]byte, 1024)
+			_, err = conn.Read(buf)
+			require.NoError(t, err)
+
+			// Close immediately without sending any commands
+			err = conn.Close()
+			require.NoError(t, err)
+
+			// Give the server time to process the disconnect
+			time.Sleep(100 * time.Millisecond)
+
+			// Verify server is still functional by connecting again
+			newConn, err := dialer.DialContext(t.Context(), "tcp", server.Addr())
+			require.NoError(t, err)
+
+			defer func() { _ = newConn.Close() }()
+
+			_, err = newConn.Read(buf)
+			require.NoError(t, err)
+			require.Contains(t, string(buf), "220")
+		})
+	}
+}
+
+// TestMultipleImmediateDisconnects tests that the server handles many rapid
+// connect/disconnect cycles gracefully (simulating probe traffic).
+func TestMultipleImmediateDisconnects(t *testing.T) {
+	t.Parallel()
+
+	server := NewTestServer(t, true)
+	dialer := &net.Dialer{Timeout: 5 * time.Second}
+
+	for range 10 {
+		conn, err := dialer.DialContext(t.Context(), "tcp", server.Addr())
+		require.NoError(t, err)
+
+		// Read welcome message
+		buf := make([]byte, 1024)
+		_, _ = conn.Read(buf)
+
+		// Immediate close
+		_ = conn.Close()
+	}
+
+	// Small delay to let server process all disconnects
+	time.Sleep(200 * time.Millisecond)
+
+	// Verify server still works
+	conf := goftp.Config{
+		User:     authUser,
+		Password: authPass,
+	}
+
+	client, err := goftp.DialConfig(conf, server.Addr())
+	require.NoError(t, err)
+
+	defer func() { _ = client.Close() }()
+
+	_, err = client.ReadDir("/")
+	require.NoError(t, err)
+}