Skip to content

Conversation

@yunzheng
Copy link
Member

When stdin is empty, raise EOFError and log this as a warning.

fixes #202

When stdin is empty, raise EOFError and log this as a warning.

fixes #202
@codecov
Copy link

codecov bot commented Jan 19, 2026

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 83.32%. Comparing base (6fab237) to head (faa6089).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
flow/record/stream.py 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #203      +/-   ##
==========================================
+ Coverage   83.25%   83.32%   +0.07%     
==========================================
  Files          35       35              
  Lines        3702     3707       +5     
==========================================
+ Hits         3082     3089       +7     
+ Misses        620      618       -2     
Flag Coverage Δ
unittests 83.32% <85.71%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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.

@JSCU-CNI
Copy link
Contributor

Could you also make the test check for the returncode @yunzheng like in #177?

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds proper handling for empty input streams when reading from stdin. When stdin is empty, the code now raises an EOFError with a descriptive message and logs it as a warning instead of failing silently or with a confusing error.

Changes:

  • Added EOFError detection and handling when peek_data is empty in RecordAdapter
  • Improved exception handling in record_stream with dedicated EOFError catch block and finally block for cleanup
  • Added comprehensive test coverage for empty stdin scenarios

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
flow/record/base.py Raises EOFError when detecting empty input stream during adapter selection
flow/record/stream.py Adds EOFError exception handling with warning log and improves resource cleanup with finally block
tests/record/test_adapter.py Adds unit test verifying EOFError is raised for empty stdin
tests/tools/test_rdump.py Adds integration tests for empty records file and empty stdin scenarios

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@yunzheng
Copy link
Member Author

Could you also make the test check for the returncode @yunzheng like in #177?

Done!

Shall we revisit #177? Or is error on empty record stream not that useful?

@JSCU-CNI
Copy link
Contributor

It seems the test from https://github.com/fox-it/flow.record/pull/177/changes#diff-4d5cab263639439a90d5ce0d7d7c87b37f5305a00d45f3d9577bc76755c959adR101-R113 does not work with the current changes in this branch.

@yunzheng
Copy link
Member Author

yunzheng commented Jan 19, 2026

It seems the test from https://github.com/fox-it/flow.record/pull/177/changes#diff-4d5cab263639439a90d5ce0d7d7c87b37f5305a00d45f3d9577bc76755c959adR101-R113 does not work with the current changes in this branch.

Can you elaborate? we did not introduce any new flags in this PR.

I ported the test that exits with an error in my last commit.

Also the stderr does not say No records were read from the input stream. But we could change the warning message if that is more clear.

@JSCU-CNI
Copy link
Contributor

JSCU-CNI commented Jan 19, 2026

Can you elaborate? we did not introduce any new flags in this PR.

The flag is indeed now irrelevant. The tests fail with input such as echo "" | rdump -v -, which does not seem to be covered by the current rdump -l test (status code should be 0 but is 1).

$ echo "" | rdump
2026-01-19T11:24:28.988179Z [error    ] rdump encountered a fatal error: Could not find adapter for file-like object [flow.record.tools.rdump]
$ echo $?
1

The tests in #177 state that the exit code should be 0 here.

@yunzheng
Copy link
Member Author

Can you elaborate? we did not introduce any new flags in this PR.

The flag is indeed now irrelevant. The tests fail with input such as echo "" | rdump -v -, which does not seem to be covered by the current rdump -l test (status code should be 0 but is 1).

note that echo "" outputs a single newline. Which is different from an empty pipe and thus should error. I made it more obvious by changing the newline to b"this is not a valid record stream"

The test with echo -n "" (empty pipe) is covered by test_rdump_empty_stdin_pipe and input=None, which i believe is the same as input=b"".

@JSCU-CNI
Copy link
Contributor

Which is different from an empty pipe and thus should error.

Agreed. Thanks for elaborating.

@yunzheng yunzheng merged commit 4bd4572 into main Jan 19, 2026
23 checks passed
@yunzheng yunzheng deleted the empty-stdin branch January 19, 2026 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Could not find adapter for file-like object for empty result

4 participants