[2.7] Mitigate F3 streaming Head-of-Line (HOL) stalls and add guardrails

[chesterxgchen]

Many Thanks to @GeorgeWang-nv to find the issues and perform the root cause analysis on this issues, suggested possible fixes

1) Root cause: connection-level Head-of-Line (HOL) blocking

• Large byte streams can freeze when a blocking socket send holds the per-connection SFM send lock.
• While that lock is held, unrelated traffic on the same connection (including progress and coordination frames) is serialized behind it.
• This creates Head-of-Line (HOL) blocking at connection level and can manifest as long apparent stalls.

2) Proposed fixes: critical must-haves, and rationale

Critical fix A: bound low-level send blocking time

• Add a bounded send timeout in the socket send path so one blocked send cannot hold the connection lock indefinitely.
• Why this way: minimally invasive, directly targets lock-hold source, and fails deterministically instead of hanging.

Critical fix B: ACK-progress watchdog in streaming sender

• Track ACK forward progress and fail fast when ACK offset does not advance for a configurable interval.
• Why this way: protects sender flow control from waiting too long on non-progressing streams, independent of generic ACK wait.

Critical fix C: SFM stalled-send detection with guarded recovery

• Monitor send stall duration in heartbeat monitor; optionally close the stalled connection to recover.
• Add a consecutive-check threshold before close to reduce false positives from transient jitter.
• Why this way: staged recovery lever (warn-only first, then auto-close) balances reliability and operational safety.

Configuration surface added

• streaming_send_timeout
• streaming_ack_progress_timeout
• streaming_ack_progress_check_interval
• sfm_send_stall_timeout
• sfm_close_stalled_connection
• sfm_send_stall_consecutive_checks

3) Test strategy and coverage

• Added focused unit tests for each mitigation path with both positive and negative cases.

Socket send timeout tests

• Config application, partial-write success, non-writable timeout, closed-send path, and closing-state suppression.

ACK watchdog tests

• ACK progress timestamp update, progress-allowed completion, no-progress timeout, legacy ack_wait behavior, and fast-fail precedence when progress timeout is small.

Stall monitor plus guard tests

• Warn-only behavior, close-enabled recovery, close-disabled non-recovery, heartbeat behavior when healthy, send-state reset on success/exception, warning emission, no-warning in healthy flow, and intermittent-stall false-alarm suppression via consecutive guard.

Validation run

• pytest on the three new F3 test modules: 24 passed.

4) User/operator guide updates

• Added a new section in docs/user_guide/timeout_troubleshooting.rst:
  • Runtime defaults for new stall-related parameters.
  • Recommended rollout: warn-only first, then auto-close only if repeated stall warnings persist.
  • Suggested values from discussion: sfm_send_stall_timeout=75, sfm_send_stall_consecutive_checks=3.
  • Log interpretation guidance and false-alarm suppression expectations.

5) Additional notes and risk profile

• This change prefers deterministic failure/recovery over silent long hangs.
• Potential side effect when auto-close is enabled: transient connection resets under severe network instability; mitigated by conservative timeout plus consecutive-check guard.
• Defaults remain conservative (sfm_close_stalled_connection=false) so operators can observe first and enable recovery when needed.
• Scope is intentionally focused on the critical Head-of-Line (HOL) mitigation path and operational guardrails.

Test plan

• python3 -m pytest tests/unit_test/fuel/f3/drivers/socket_conn_timeout_test.py tests/unit_test/fuel/f3/streaming/byte_streamer_ack_watchdog_test.py tests/unit_test/fuel/f3/sfm/sfm_stall_monitor_test.py
• python3 -m black <changed_python_files>
• python3 -m isort <changed_python_files>
• python3 -m flake8 <changed_python_files>

Latest Timeout Hardening Update

• Close socket connections on send_frame timeout to prevent frame-boundary desync after partial writes.
• Preserve specific send error codes in send_frame for common socket exceptions:
  • map timeout-like exceptions to CommError.TIMEOUT (and close connection),
  • map closed-socket exceptions to CommError.CLOSED,
  • keep CommError.ERROR as fallback for unknown exceptions.
• Add positive and negative unit tests in socket_conn_timeout_test.py:
  • Positive: timeout after partial write closes the connection.
  • Positive: TimeoutError -> TIMEOUT; BrokenPipeError -> CLOSED.
  • Negative: successful partial-write sends do not close; non-timeout CommError.CLOSED does not force close; unknown exceptions remain ERROR.
• Document stall timing relationship in timeout_troubleshooting.rst, including the practical close window formula and outer-timeout sizing guideline.

Validation

• python3 -m pytest tests/unit_test/fuel/f3/drivers/socket_conn_timeout_test.py -q
• Result: 10 passed

[greptile-apps]

Greptile Summary

This PR implements three complementary mitigation strategies to address Head-of-Line (HOL) blocking in F3 streaming, where large byte streams could freeze when a blocking socket send holds the per-connection lock. The implementation includes bounded send timeouts with connection close on partial-write timeouts (to prevent frame desync), an ACK progress watchdog in the streaming sender (to fail fast when ACK offset stops advancing), and an SFM stall monitor with consecutive-check guards (to detect and optionally close stalled connections). All three fixes work together to convert silent hangs into deterministic failures with recovery options.

Key Changes:

• Socket send timeout with select-based flow control closes connection after partial writes to avoid frame-boundary desync
• ACK progress tracking with configurable timeout fails fast when stream makes no progress, independent of generic ACK wait
• Heartbeat monitor detects send stalls and can optionally close connections after consecutive threshold checks to reduce false positives
• Conservative defaults (sfm_close_stalled_connection=false) enable warn-only mode for safe initial deployment
• Comprehensive unit tests (24 tests) cover timeout paths, watchdog behavior, and consecutive-check guards
• Documentation includes deployment guidelines, timing relationships, and log interpretation guidance

Confidence Score: 5/5

• This PR is safe to merge with minimal risk
• The implementation addresses a critical production issue (HOL blocking) with well-designed, minimally invasive fixes. Conservative defaults ensure safe rollout (warn-only mode), comprehensive test coverage validates all mitigation paths, and the consecutive-check guard reduces false positives. The previous review concern about error code preservation has been addressed.
• No files require special attention

Important Files Changed

Filename	Overview
nvflare/fuel/f3/comm_config.py	Adds configuration parameters for streaming timeout and stall detection features with appropriate getter methods
nvflare/fuel/f3/drivers/socket_conn.py	Implements bounded socket send timeout with select-based flow control and connection close on timeout to prevent frame desync
nvflare/fuel/f3/sfm/sfm_conn.py	Adds send state tracking with monotonic timestamps to enable stall detection by heartbeat monitor
nvflare/fuel/f3/streaming/byte_streamer.py	Adds ACK progress watchdog that fails fast when offset does not advance within configured timeout
nvflare/fuel/f3/sfm/heartbeat_monitor.py	Adds send stall detection with consecutive-check guard and optional connection close for recovery
docs/user_guide/timeout_troubleshooting.rst	Comprehensive documentation for streaming stall guardrails with recommended deployment guidelines and timing relationships

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Large Byte Stream Send] --> B{Socket Send Blocks}
    B --> C[Holds SFM Connection Lock]
    C --> D[HOL Blocking: Other Traffic Stalled]
    
    D --> E[Fix A: Bounded Send Timeout]
    E --> E1[select with timeout]
    E1 --> E2{Timeout After Partial Write?}
    E2 -->|Yes| E3[Close Connection<br/>Prevent Frame Desync]
    E2 -->|No| E4[Complete Send Successfully]
    
    D --> F[Fix B: ACK Progress Watchdog]
    F --> F1[Track last_ack_progress_ts]
    F1 --> F2{ACK Offset Advancing?}
    F2 -->|No Progress| F3[Fail Fast After<br/>ack_progress_timeout]
    F2 -->|Progress| F4[Allow Stream to Continue]
    
    D --> G[Fix C: SFM Stall Monitor]
    G --> G1[Heartbeat Monitor Checks<br/>send_stall_seconds]
    G1 --> G2{Stall > Timeout?}
    G2 -->|Yes| G3[Increment Consecutive Count]
    G3 --> G4{Count >= Threshold?}
    G4 -->|Yes & Close Enabled| G5[Close Connection<br/>Force Recovery]
    G4 -->|No or Close Disabled| G6[Warn Only]
    G2 -->|No| G7[Reset Counter to 0]

Loading

_{Last reviewed commit: a6a4253}

[greptile-apps]

_{10 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

_{10 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[chesterxgchen]

/build

[chesterxgchen]

/build

[greptile-apps]

_{10 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[pcnudde]

I'm ok with this approach. It goes a bit further then PR 4205, but still is safe. only a few comments

[greptile-apps]

_{12 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

_{12 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[chesterxgchen]

/build

[greptile-apps]

_{10 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[chesterxgchen]

/build

[chesterxgchen]

/build

[greptile-apps]

_{10 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

[chesterxgchen]

/build

[greptile-apps]

_{10 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}