fix(printer): properly close output files to prevent data loss #4998
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a critical bug where printer Close() methods were not closing their underlying output files, causing intermittent data loss when write buffers weren't flushed before process termination.
Key Changes:
- Implemented proper file closing logic in Close() methods for three printer types (tableEventPrinter, templateEventPrinter, jsonEventPrinter)
- Added nil checks and error logging when closing output files
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4998 +/- ##
=======================================
Coverage ? 29.69%
=======================================
Files ? 234
Lines ? 26086
Branches ? 0
=======================================
Hits ? 7745
Misses ? 17802
Partials ? 539
🚀 New features to boost your workflow:
|
yanivagman
force-pushed
the
fix_intermittent_test_failures
branch
from
f58226f to
75d2681
Compare
Printer Close() methods now call Sync() to flush OS buffers before process exit. This fixes intermittent test failures where buffered events weren't written to disk during graceful shutdown.
yanivagman
force-pushed
the
fix_intermittent_test_failures
branch
from
75d2681 to
dbe2a1c
Compare
| assert.Contains(t, output, `1234567890`, "timestamp should be included") | ||
| } | ||
|
|
||
| // TestPrinterCloseFlusheData tests that Close() calls Sync() to flush buffered data to disk |
There was a problem hiding this comment.
Corrected spelling of 'FlushesData' in function name (typo in comment: 'FlusheData').
| // TestPrinterCloseFlusheData tests that Close() calls Sync() to flush buffered data to disk | |
| // TestPrinterCloseFlushesData tests that Close() calls Sync() to flush buffered data to disk |
|
|
||
| func (p tableEventPrinter) Close() { | ||
| // Sync flushes buffered data, ensuring events aren't lost on process exit | ||
| if f, ok := p.out.(*os.File); ok && f != nil { |
There was a problem hiding this comment.
The nil check f != nil is redundant after a successful type assertion. If the type assertion succeeds, f cannot be nil. Consider simplifying to if f, ok := p.out.(*os.File); ok {.
| if f, ok := p.out.(*os.File); ok && f != nil { | |
| if f, ok := p.out.(*os.File); ok { |
|
|
||
| func (p templateEventPrinter) Close() { | ||
| // Sync flushes buffered data, ensuring events aren't lost on process exit | ||
| if f, ok := p.out.(*os.File); ok && f != nil { |
There was a problem hiding this comment.
The nil check f != nil is redundant after a successful type assertion. If the type assertion succeeds, f cannot be nil. Consider simplifying to if f, ok := p.out.(*os.File); ok {.
| if f, ok := p.out.(*os.File); ok && f != nil { | |
| if f, ok := p.out.(*os.File); ok { |
|
|
||
| func (p jsonEventPrinter) Close() { | ||
| // Sync flushes buffered data, ensuring events aren't lost on process exit | ||
| if f, ok := p.out.(*os.File); ok && f != nil { |
There was a problem hiding this comment.
The nil check f != nil is redundant after a successful type assertion. If the type assertion succeeds, f cannot be nil. Consider simplifying to if f, ok := p.out.(*os.File); ok {.
| if f, ok := p.out.(*os.File); ok && f != nil { | |
| if f, ok := p.out.(*os.File); ok { |
2 participants
The Close() methods for jsonEventPrinter, templateEventPrinter, and tableEventPrinter were empty stubs that never closed the underlying io.WriteCloser. This caused intermittent test failures when OS write buffers weren't flushed before process termination, resulting in lost events in the output files.
Now all printer types properly close their output files, ensuring buffers are flushed and all events are written to disk.