fix: prevent IdleTimeout from closing control connection during activ…

[fclairamb]

…e data transfers

Before this change, the server would immediately close control connections when IdleTimeout was reached, regardless of whether a data transfer was active. This caused issues with large or slow file transfers that could take longer than the configured idle timeout.

Now the server checks if there is an active data transfer (isTransferOpen) before closing the connection. If a transfer is active, it extends the deadline instead of closing the connection, allowing the transfer to complete.

Fixes #430

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 33.33333% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.40%. Comparing base (8ca225b) to head (25b233a).

Files with missing lines	Patch %	Lines
client_handler.go	33.33%	9 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #577      +/-   ##
==========================================
- Coverage   86.74%   86.40%   -0.35%     
==========================================
  Files          12       12              
  Lines        2369     2383      +14     
==========================================
+ Hits         2055     2059       +4     
- Misses        238      247       +9     
- Partials       76       77       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[claude]

Code Review for PR #577

Summary

This PR addresses issue #430 by preventing the control connection from being closed due to idle timeout when there's an active data transfer in progress. The solution is straightforward and correctly identifies active transfers before deciding whether to close the connection.

✅ Strengths

• Correct fix: The solution properly addresses the core issue where large/slow transfers could be interrupted by idle timeout
• Thread-safe: Uses proper mutex locking when checking isTransferOpen state
• Minimal change: Focused fix that doesn't introduce unnecessary complexity
• Follows existing patterns: Uses the same deadline extension pattern already present in the codebase

🔍 Issues and Recommendations

1. Potential Infinite Loop Risk 🚨

The current implementation extends the deadline by IdleTimeout seconds each time. If a transfer stalls (e.g., network issues, client hanging), this could keep the connection alive indefinitely.

Recommendation: Consider adding a maximum transfer timeout or counter to prevent infinite extensions:

// Add a field to track consecutive extensions
if hasActiveTransfer {
    if c.transferTimeoutExtensions >= maxTransferExtensions {
        // Force close after too many extensions
        c.logger.Warn("Transfer timeout exceeded maximum extensions")
        // Continue with normal timeout handling
    } else {
        c.transferTimeoutExtensions++
        // Extend deadline...
    }
}

2. Missing Test Coverage ⚠️

There are no tests for this new behavior. The existing TestIdleTimeout doesn't cover the scenario where a transfer is active.

Recommendation: Add a test case that:

• Initiates a slow transfer (using throttled connection or large file)
• Verifies the control connection remains open beyond IdleTimeout
• Confirms connection closes after transfer completes

3. Deadline Calculation Complexity 🔧

The deadline calculation is unnecessarily complex:

time.Duration(time.Second.Nanoseconds() * int64(c.server.settings.IdleTimeout))

Simplification:

time.Duration(c.server.settings.IdleTimeout) * time.Second

4. Reset Transfer Extension Counter 📝

When a transfer completes successfully, any transfer extension counter should be reset for the next transfer (in the closeTransfer method at client_handler.go:339).

🔒 Security Considerations

• The fix doesn't introduce new attack vectors
• However, consider adding rate limiting or maximum transfer duration to prevent resource exhaustion attacks where clients intentionally keep slow transfers running

📊 Performance Impact

• Minimal overhead: One mutex lock/unlock per timeout event during active transfers
• No impact on normal operations without active transfers

📚 Documentation

Consider updating documentation to clarify:

• How IdleTimeout behaves during active transfers
• Any maximum transfer duration limits (if implemented)

✅ Overall Assessment

The PR correctly fixes the reported issue with a clean, focused change. However, it needs:

1. Test coverage for the new behavior
2. Protection against infinite deadline extensions
3. Minor code simplification for readability

Once these items are addressed, this will be a solid fix for the idle timeout issue during active transfers.

[claude]

Code Review for PR #577: Fix IdleTimeout during active transfers

Thank you for this fix! I've reviewed the changes and have the following feedback:

✅ Strengths

• Correctly addresses the issue: The fix properly checks for active data transfers before closing the control connection on idle timeout
• Thread-safe implementation: Proper use of mutex lock/unlock to access isTransferOpen
• Maintains existing behavior: When no transfer is active, the original timeout handling is preserved
• Good defensive coding: Includes error handling for SetDeadline failures

🔍 Areas for Improvement

1. Deadline Extension Logic

The current implementation has a potential issue with the deadline extension calculation:

extendedDeadline := time.Now().Add(time.Duration(time.Second.Nanoseconds() * int64(c.server.settings.IdleTimeout)))

Issues:

• The conversion is unnecessarily complex. Since IdleTimeout is already in seconds, you can simplify this to:

  extendedDeadline := time.Now().Add(time.Duration(c.server.settings.IdleTimeout) * time.Second)

• More importantly, this creates a recursive timeout loop. If the transfer takes longer than another idle timeout period, the same timeout will trigger again, extending indefinitely. Consider using a longer extension period or a maximum number of extensions.

2. Missing Test Coverage

The PR doesn't include tests for this new behavior. Consider adding a test case that:

• Sets up a server with a short idle timeout
• Initiates a large file transfer that takes longer than the idle timeout
• Verifies the connection stays open during the transfer
• Confirms the connection closes after the transfer completes and idle timeout expires

Example test structure:

func TestIdleTimeoutWithActiveTransfer(t *testing.T) {
    // Create server with 2-second idle timeout
    // Start a file transfer
    // Sleep for 3 seconds (> idle timeout)
    // Verify connection is still alive
    // Complete transfer
    // Wait for idle timeout
    // Verify connection closes
}

3. Potential Race Condition

While the mutex protects isTransferOpen, there's still a window between checking the flag and extending the deadline where the transfer could complete. This is likely acceptable given the use case, but worth noting.

4. Consider Alternative Approaches

Instead of repeatedly extending on timeout, consider:

• Disabling idle timeout entirely while a transfer is active (set to 0 or very large value when transfer starts, restore when it ends)
• Using a separate goroutine to monitor transfer status and manage deadlines
• Implementing a transfer-aware timeout that resets on data activity rather than control channel activity

📝 Suggested Implementation

Here's a more robust approach:

if hasActiveTransfer {
    // Extend by a reasonable period (e.g., 5 minutes or 10x idle timeout)
    extensionPeriod := time.Duration(c.server.settings.IdleTimeout) * time.Second * 10
    if extensionPeriod < 5*time.Minute {
        extensionPeriod = 5 * time.Minute
    }
    
    if errSet := c.conn.SetDeadline(time.Now().Add(extensionPeriod)); errSet != nil {
        c.logger.Error("Could not extend deadline during transfer", "err", errSet)
        // Consider whether to continue or close the connection here
    }
    
    if c.debug {
        c.logger.Debug("Extended deadline due to active transfer", 
            "extension", extensionPeriod)
    }
    return
}

🚀 Overall Assessment

This is a good fix for an important issue. With the suggested improvements to the deadline calculation and added test coverage, this will be a solid addition to the codebase. The fix aligns well with the library's architecture and maintains backward compatibility.

[claude]

Code Review for PR #577

I've reviewed the changes to fix the IdleTimeout issue during active data transfers. Here's my feedback:

✅ Positive Aspects

1. Correct Problem Identification: The fix correctly addresses issue IdleTimeout doesn't factor in active data connection (large or slow transfers) #430 by checking for active transfers before closing the control connection on idle timeout.

2. Proper Mutex Usage: The code correctly uses transferMu.Lock() and Unlock() to safely read isTransferOpen, following the existing pattern in the codebase (client_handler.go:99).

3. Appropriate Logging: Debug logging is added to track when deadlines are extended during active transfers.

⚠️ Issues & Concerns

1. Critical Bug: Inconsistent Transfer State Management (client_handler.go:339, 676)

The isTransferOpen flag is not protected by transferMu when it's set to true (line 676) or false (line 339) in other parts of the code. This creates a race condition:

• Line 676: c.isTransferOpen = true (no lock held)
• Line 339: c.isTransferOpen = false (no lock held)
• PR adds: Lines 479-481 check isTransferOpen (lock held)

Recommendation: All reads AND writes to isTransferOpen must be protected by transferMu to prevent data races. The existing code at lines 339 and 676 should also acquire the lock.

2. Incorrect Time Calculation (Line 486)

extendedDeadline := time.Now().Add(time.Duration(time.Second.Nanoseconds() * int64(c.server.settings.IdleTimeout)))

This is unnecessarily complex and error-prone. IdleTimeout is already in seconds, so you're doing: time.Second.Nanoseconds() * seconds = nanoseconds, then converting back to time.Duration.

Recommendation: Simplify to:

extendedDeadline := time.Now().Add(time.Duration(c.server.settings.IdleTimeout) * time.Second)

3. Missing Test Coverage

There's an existing TestIdleTimeout (handle_misc_test.go:44) but no test that validates:

• Idle timeout doesn't close connection during active transfer
• Deadline is properly extended during transfers
• Normal idle timeout still works when no transfer is active

Recommendation: Add a test case like:

func TestIdleTimeoutDuringTransfer(t *testing.T) {
    // Setup server with short idle timeout
    // Start a slow file transfer
    // Verify control connection stays open past idle timeout
    // Verify deadline is extended (check logs or add metrics)
}

4. Duplicate Deadline Extension Logic (Lines 480, 486)

The code extends the deadline twice in the same error path:

• Line 480: c.conn.SetDeadline(time.Now().Add(time.Minute)) (existing code)
• Line 486: Sets deadline to IdleTimeout (new code, only when transfer active)

Recommendation: The logic flow seems confusing. Consider restructuring to:

if errNetError.Timeout() {
    c.transferMu.Lock()
    hasActiveTransfer := c.isTransferOpen
    c.transferMu.Unlock()

    if hasActiveTransfer {
        // Extend deadline for active transfer
        extendedDeadline := time.Now().Add(time.Duration(c.server.settings.IdleTimeout) * time.Second)
        if errSet := c.conn.SetDeadline(extendedDeadline); errSet != nil {
            c.logger.Error("Could not extend deadline during active transfer", "err", errSet)
        }
        if c.debug {
            c.logger.Debug("Idle timeout during active transfer, extending deadline")
        }
        return
    }

    // No active transfer, proceed with timeout handling
    if errSet := c.conn.SetDeadline(time.Now().Add(time.Minute)); errSet != nil {
        c.logger.Error("Could not set read deadline", "err", errSet)
    }
    // ... rest of timeout handling
}

🔒 Security Considerations

Potential DoS Vector: An attacker could keep a connection open indefinitely by continuously initiating transfers. The deadline is extended by IdleTimeout each time, so with a 60-second timeout and repeated transfers, a connection could stay open forever.

Recommendation: Consider adding:

• Maximum total connection duration (separate from idle timeout)
• Transfer timeout (maximum time for a single transfer)
• Limit on consecutive deadline extensions

📊 Performance Considerations

The mutex lock/unlock is lightweight and happens only during timeout events (rare). Performance impact is negligible.

📋 Summary

Must Fix Before Merge:

1. Fix race condition by protecting ALL isTransferOpen access with transferMu
2. Simplify time calculation

Should Fix:

1. Add test coverage for this scenario
2. Clarify deadline extension logic
3. Consider DoS protection

Code Quality: 6/10 (correct concept, implementation needs refinement)

🤖 Generated with Claude Code

[claude]

Pull Request Review: Fix IdleTimeout During Active Transfers

Thank you for addressing issue #430! This is an important fix for handling large or slow file transfers.

Positive Aspects

1. Correct Problem Identification: The fix correctly identifies that the control connection timeout should not terminate ongoing data transfers.
2. Thread-Safe Access: Properly uses transferMu mutex to safely access isTransferOpen.
3. Appropriate Logging: Includes debug logging when extending the deadline during active transfers.
4. Minimal Impact: The change is localized and doesn't affect other code paths.

Issues and Concerns

1. Logic Flow Problem (client_handler.go:479-493)

After the new code block that extends the deadline and returns early, the original timeout handling code still executes when there's NO active transfer. This code path extends the deadline by 1 minute (line 480-481) but then immediately logs timeout and notifies the client (line 484-491). Why extend the deadline only to immediately close the connection? This seems like a legacy quirk - the 1-minute extension appears necessary only to allow the error message to be sent.

2. Inconsistent Deadline Calculation

The new code uses: time.Duration(time.Second.Nanoseconds() * int64(c.server.settings.IdleTimeout))
The existing code at line 441 uses the same complex calculation
The original fallback at line 480 uses simple time.Minute

Recommendation: Consider extracting a helper method to make this consistent and clearer.

3. Potential Race Condition Window

There's a brief window between checking isTransferOpen and extending the deadline where the transfer could complete. While unlikely to cause issues in practice, the worst case is an extra deadline extension which is benign.

4. Missing Test Coverage

The existing TestIdleTimeout (handle_misc_test.go:44) doesn't cover the new behavior. A test case should verify that the control connection remains open during active data transfers that exceed IdleTimeout.

Suggested Improvements

1. Add test coverage for the new behavior
2. Add documentation comment explaining the timeout extension logic during active transfers
3. Consider refactoring the deadline calculation into a helper method (optional but recommended)

Security Considerations

Potential DoS Vector: A malicious client could keep a data connection open indefinitely to bypass idle timeout. However, this is mitigated by the ConnectionTimeout setting for data connections, and existing behavior already allows this via legitimate slow transfers. No new security concerns are introduced.

Verdict

Conditionally Approve - The fix correctly addresses the reported issue and is safe to merge for fixing the immediate problem. The suggested improvements would enhance maintainability and test coverage.

---

Review generated with assistance from Claude Code

[claude]

Pull Request Review: Fix IdleTimeout During Active Transfers

Summary

This PR addresses issue #430 by preventing the IdleTimeout from closing the control connection when a data transfer is active. The approach is sound and directly solves the reported problem.

---

Code Quality & Best Practices

✅ Strengths:

• Clean use of existing transferMu mutex to safely read isTransferOpen state
• Proper lock scoping (lock/unlock pattern is correct)
• Follows the existing code style and conventions
• Debug logging is appropriately conditional

⚠️ Issues Identified:

1. Time Duration Calculation (Line 486)

   extendedDeadline := time.Now().Add(time.Duration(time.Second.Nanoseconds() * int64(c.server.settings.IdleTimeout)))

   This is overly complex. The existing code at line 440-441 uses the same pattern, but it can be simplified to:

   extendedDeadline := time.Now().Add(time.Duration(c.server.settings.IdleTimeout) * time.Second)

   This is more idiomatic Go and clearer in intent.

2. Code Duplication
   The deadline extension logic now exists in two places:

   • Line 440-441 (before reading commands)
   • Line 486 (new code for active transfers)

   Consider extracting to a helper method like getIdleDeadline().

3. Line Length
   Line 486 exceeds the project's 120-character maximum enforced by linter (as stated in CLAUDE.md).

---

Potential Bugs & Issues

🔴 Critical: Infinite Loop Risk

The current implementation has a significant flaw: when a transfer is active, it extends the deadline and returns immediately, but the main command loop (client_handler.go:440) will immediately set the deadline again before the next ReadLine() call.

Scenario:

1. Transfer starts, isTransferOpen = true
2. IdleTimeout triggers (no commands during transfer)
3. Your code extends deadline and returns
4. Loop continues to line 440, sets deadline to Now() + IdleTimeout
5. If transfer takes longer than IdleTimeout, step 2 repeats indefinitely

This creates a busy loop that repeatedly wakes up every IdleTimeout seconds while the transfer is active, which is inefficient.

Recommended Fix:
Extend the deadline by a longer duration when a transfer is active, not just IdleTimeout. For example:

// Give more time for transfers - extend by IdleTimeout rather than setting to IdleTimeout
extendedDeadline := time.Now().Add(time.Duration(c.server.settings.IdleTimeout) * time.Second * 2)

Or better yet, extend by a multiple of IdleTimeout or use a maximum transfer timeout setting.

---

Race Conditions

✅ Thread Safety:
The mutex usage is correct. However, there's a subtle TOCTOU (time-of-check-time-of-use) race:

1. Thread A: Reads isTransferOpen=true, releases lock
2. Thread B: Sets isTransferOpen=false (transfer completes)
3. Thread A: Extends deadline for a completed transfer

Impact: Low - worst case is one extra IdleTimeout extension after transfer completes. Not critical, but worth noting.

Mitigation (if needed): Hold the lock while setting the deadline, though this adds minimal value given the low impact.

---

Performance Considerations

1. Lock Contention: Minimal - the lock is held briefly to read a boolean
2. Busy Loop: As mentioned above, this could cause repeated timeout handling during long transfers
3. No Performance Regression: The check only adds ~3 instructions to the timeout path

---

Security Concerns

⚠️ Potential DoS Vector:

An attacker could potentially keep a connection alive indefinitely by:

1. Starting a data transfer (isTransferOpen=true)
2. Sending data very slowly (never completing the transfer)
3. The control connection never times out

Recommendation:
Consider adding a maximum transfer duration separate from IdleTimeout. For example:

type Settings struct {
    IdleTimeout         int // Control connection idle timeout
    MaxTransferDuration int // Maximum time for a single transfer (0 = unlimited)
}

This would allow long transfers while preventing infinite connections.

---

Test Coverage

❌ Missing Tests:

The PR does not include any tests. Given that:

• handle_misc_test.go:44-72 already has TestIdleTimeout
• Issue IdleTimeout doesn't factor in active data connection (large or slow transfers) #430 specifically mentions this bug
• The comment at line 40-43 references a similar issue from 2020

Required:
Add a test case like:

func TestIdleTimeoutDuringTransfer(t *testing.T) {
    // Setup server with short IdleTimeout
    // Start a file transfer
    // Verify control connection stays alive during transfer
    // Verify control connection times out after transfer completes + IdleTimeout
}

This is critical for preventing regression.

---

Recommended Changes

High Priority:

1. Fix the deadline extension to use a longer duration (multiple of IdleTimeout)
2. Add test coverage for the new behavior
3. Simplify the time duration calculation

Medium Priority:
4. Consider extracting deadline calculation to a helper method
5. Add maximum transfer duration to prevent DoS

Low Priority:
6. Document the new behavior in comments or README

---

Overall Assessment

Verdict: ⚠️ Needs Revision

The PR correctly identifies and addresses the root cause, but the implementation needs refinement to avoid creating a busy loop during transfers. With the suggested fixes and test coverage, this will be a solid improvement.

Estimated Risk: Medium (without fixes), Low (with fixes)

---

🤖 Generated with Claude Code

[claude]

Pull Request Review

Thank you for addressing issue #430! This is an important fix for handling idle timeouts during active data transfers. Here's my comprehensive review:

✅ Code Quality & Best Practices

Strengths:

• Proper mutex usage: Correctly using transferMu.Lock()/Unlock() to protect access to isTransferOpen
• Follows existing patterns: Consistent with the codebase's locking conventions (see client_handler.go:404, 641, 692)
• Clear logic flow: Easy to understand - check for active transfer before terminating connection
• Appropriate logging: Good use of debug logging to track the deadline extension

Minor style notes:

• Code adheres to the project's 120-character line length limit ✓
• Follows Go conventions and error handling patterns ✓

🐛 Potential Issues & Concerns

1. Race Condition Risk (Medium Priority)

The current implementation has a potential check-then-act race condition:

c.transferMu.Lock()
hasActiveTransfer := c.isTransferOpen
c.transferMu.Unlock()

if hasActiveTransfer {
    // Between unlock and here, transfer could close
    extendedDeadline := time.Now().Add(...)
    c.conn.SetDeadline(extendedDeadline)
}

Issue: After unlocking transferMu, the transfer could complete and isTransferOpen could become false before you extend the deadline. This could lead to unnecessarily extending the deadline for a closed transfer.

Recommendation: Hold the lock during the SetDeadline call, or use an atomic operation. Example:

c.transferMu.Lock()
if c.isTransferOpen {
    extendedDeadline := time.Now().Add(...)
    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")
    }
    c.transferMu.Unlock()
    return
}
c.transferMu.Unlock()

However, since c.conn.SetDeadline() is a network operation, holding the lock might not be ideal. Consider the trade-offs.

2. Deadline Calculation Duplication (Low Priority)

The deadline calculation is duplicated:

• Line 441 (existing): time.Duration(time.Second.Nanoseconds() * int64(c.server.settings.IdleTimeout))
• PR addition: Same calculation

Recommendation: Extract to a helper method or reuse the same calculation pattern. This improves maintainability.

3. Incomplete Fix - Original Timeout Logic Still Executes (High Priority)

CRITICAL ISSUE: After the new code returns early, the original timeout handling code at lines 479-493 still executes in the non-transfer case. However, looking at the diff, the new code is inserted before the existing timeout handling. This means:

1. ✅ If transfer is active → extends deadline and returns (good!)
2. ✅ If no transfer → falls through to existing logic (good!)

Actually, this is correct! The early return prevents the connection closure message. My apologies for the initial confusion.

⚡ Performance Considerations

• Mutex contention: Minimal - the lock is held briefly only to read a boolean
• SetDeadline overhead: Network syscall, but necessary and proportional to idle timeouts (not a hot path)
• No busy waiting or polling: Excellent! Event-driven approach ✓

🔒 Security Concerns

Potential DoS vector (Low-Medium Risk):

• An attacker could keep a transfer connection open indefinitely to prevent idle timeout on the control connection
• However, this is somewhat mitigated by:
  1. Transfer connections themselves should have their own timeouts
  2. This is the expected behavior for legitimate long-running transfers
  3. Resource limits should be handled at a higher level (max connections, etc.)

Recommendation: Document this behavior and ensure transfer connections have appropriate timeouts configured elsewhere in the codebase. This is likely already handled but worth verifying.

🧪 Test Coverage

Current state:

• Existing test TestIdleTimeout (line 44 in handle_misc_test.go) tests basic timeout functionality
• Missing: No test covering the new code path (timeout during active transfer)

Recommendation: Add a test case that:

1. Starts a long-running transfer (e.g., large file or slow reader)
2. Verifies control connection stays alive beyond IdleTimeout during transfer
3. Verifies control connection times out normally after transfer completes + idle period

Example test outline:

func TestIdleTimeoutDuringActiveTransfer(t *testing.T) {
    server := NewTestServerWithTestDriver(t, &TestServerDriver{
        Debug: false, 
        Settings: &Settings{IdleTimeout: 2},
    })
    // 1. Connect and authenticate
    // 2. Start a STOR/RETR that takes longer than IdleTimeout
    // 3. Wait IdleTimeout + 1 second
    // 4. Verify control connection is still alive (send NOOP)
    // 5. Complete transfer
    // 6. Wait IdleTimeout + 1 second  
    // 7. Verify control connection now times out
}

📋 Summary

Verdict: ✅ Approve with recommendations

This PR correctly addresses the core issue from #430. The fix is conceptually sound and follows the codebase conventions well.

Required before merge:

1. ⚠️ Consider the race condition mentioned above and potentially adjust the locking strategy
2. ⚠️ Add test coverage for the new code path

Optional improvements:
3. Extract deadline calculation to a helper method
4. Document the intentional behavior regarding transfers preventing timeout
5. Verify transfer connection timeout handling elsewhere in the codebase

Great work on identifying and fixing this issue! The comments in the existing code (handle_misc_test.go:40-43) show this has been a known limitation since 2020, so this is a valuable contribution.

---

Review generated with analysis of code patterns, concurrency safety, and FTP protocol requirements