fix fd leak in GenerateDataFromFile on send error

[cyyever]

Description

This PR fixes fd leak in GenerateDataFromFile on send error.

Types of changes

• Non-breaking change (fix or new feature that would not break existing functionality).
• Breaking change (fix or new feature that would cause existing functionality to change).
• New tests added to cover the changes.
• Quick tests passed locally by running ./runtest.sh.
• In-line docstrings updated.
• Documentation updated.

[greptile-apps]

Greptile Summary

This PR fixes a file-descriptor leak in GenerateDataFromFile where the fd was only closed inside generate() on EOF, leaving it open whenever sender.sendall() raised mid-stream. The fix wraps send_binary_data in closing(generator), moves cleanup to a new close() hook on DataGenerator, and also replaces os.stat(file_name) with os.fstat(self.file.fileno()) to eliminate a TOCTOU window.

• DataGenerator gains an optional, no-op close() override; GenerateDataFromFile implements it as self.file.close(), which is idempotent and safe for repeated calls.
• Three new unit tests verify the fd is closed on success, on a sender exception, and on a body-size mismatch — covering all exit paths of send_binary_data.

Confidence Score: 5/5

Safe to merge — targeted resource-cleanup fix with no behavioural side effects on the happy path.

The refactor only adds a guaranteed-close guarantee via closing(generator) and replaces the in-generate() close with a dedicated close() hook; all existing send/receive logic is preserved. The three new tests exercise every exit path of send_binary_data and confirm the fd is closed in each case.

No files require special attention.

Important Files Changed

Filename	Overview
nvflare/fuel/hci/binary_proto.py	Adds close() hook to DataGenerator base class, wraps send_binary_data body in closing(generator), and refactors GenerateDataFromFile to close the file via close() instead of inside generate(); also swaps os.stat for os.fstat to avoid a TOCTOU race.
tests/unit_test/fuel/hci/binary_proto_test.py	Adds three targeted tests covering fd-close behaviour on success, on mid-stream sender failure, and on body-size mismatch; uses FailingSender helper to inject failures at a configurable call index.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant send_binary_data
    participant closing ctx
    participant GenerateDataFromFile
    participant Sender

    Caller->>send_binary_data: send_binary_data(sender, generator, meta)
    send_binary_data->>closing ctx: __enter__(generator)
    send_binary_data->>GenerateDataFromFile: data_size()
    send_binary_data->>Sender: sendall(BINARY_MARKER)
    send_binary_data->>Sender: sendall(header_bytes)
    send_binary_data->>Sender: sendall(meta_bytes)
    loop until generate() returns empty
        send_binary_data->>GenerateDataFromFile: generate()
        GenerateDataFromFile-->>send_binary_data: chunk
        send_binary_data->>Sender: sendall(chunk)
        note over Sender: raises IOError on failure
    end
    send_binary_data->>Sender: sendall(footer_bytes)
    closing ctx->>GenerateDataFromFile: close() [always, even on exception]
    GenerateDataFromFile->>GenerateDataFromFile: self.file.close()
    send_binary_data-->>Caller: sent_body_size

Loading

_{Reviews (4): Last reviewed commit: "Merge branch 'main' into fix/binary-prot..." | Re-trigger Greptile}

[pcnudde]

if commits are not signed we can not merge them.

[cyyever]

@pcnudde Signed it.

[chesterxgchen]

1. One observation: test_fd_closed_on_body_size_mismatch mutates gen.size = 999_999 after construction to
   fake a mismatch. This works and is acceptable for this kind of unit test, but it's relying on
   implementation internals. A comment noting why would help future readers (# force size mismatch to
   exercise the post-loop RuntimeError path).

The suggestion is a one-line comment in test_fd_closed_on_body_size_mismatch to explain the gen.size
mutation.

2. nvflare/fuel/hci/binary_proto.py:394: with closing(generator) makes send_binary_data() require every generator object to implement close(). Before this PR, the runtime contract was effectively duck-
   typed: anything with data_size() and generate() worked. A custom generator that does not subclass DataGenerator can now successfully send the full payload, then fail on exit with AttributeError: ... has no attribute 'close'. To preserve compatibility, use a try/finally and call close() only when present:

   close = getattr(generator, "close", None)
   try:
   ...
   finally:
   if close:
   close()