out_file: add logs rotation feature #11352
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds size-based file rotation and optional streaming gzip compression to the out_file plugin, with per-file size tracking and locking, rotation/retention controls, init/flush integration, and a comprehensive runtime test suite for rotation behaviors and formats. Changes
Sequence Diagram(s)sequenceDiagram
participant FluentBit
participant cb_file_flush
participant SizeRegistry
participant Rotation
participant Gzip
participant Filesystem
FluentBit->>cb_file_flush: flush(records, filename)
cb_file_flush->>SizeRegistry: find/create entry(filename) & lock
cb_file_flush->>Filesystem: ensure directory & write formatted records
cb_file_flush->>SizeRegistry: increment entry.size
alt rotation enabled and entry.size >= max_size
cb_file_flush->>Rotation: rotate_file(filename, entry)
Rotation->>Filesystem: rename current -> filename.timestamp
alt gzip enabled
Rotation->>Gzip: gzip_compress_file(timestamped -> .gz)
Gzip->>Filesystem: write .gz and remove original
end
Rotation->>Filesystem: cleanup_old_files(directory, base_filename)
Rotation->>SizeRegistry: reset entry.size
end
cb_file_flush->>SizeRegistry: unlock
cb_file_flush->>FluentBit: return status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-08-31T12:46:11.940ZApplied to files:
📚 Learning: 2025-10-23T07:43:16.216ZApplied to files:
🧬 Code graph analysis (1)tests/runtime/out_file_logrotate.c (3)
🪛 ast-grep (0.40.5)tests/runtime/out_file_logrotate.c[warning] 619-619: A check is done with (file-stat-before-action-c) 🔇 Additional comments (12)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 628922c6a1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @plugins/out_file/file.c:
- Around line 222-238: The error paths after flb_lock_init(&ctx->list_lock) do
not destroy the lock before freeing ctx; update the failure branches for
flb_output_config_map_set and the ctx->max_files validation to call
flb_lock_destroy(&ctx->list_lock) (or the appropriate destroy function) prior to
flb_free(ctx) and returning -1 so that the lock is cleaned up; locate these
spots around the mk_list_init, flb_lock_init, flb_output_config_map_set, and
flb_plg_error calls and add the destroy call in both error branches.
🧹 Nitpick comments (2)
tests/runtime/out_file_logrotate.c (2)
1040-1041: Array oversized for declared usage.
pthread_t threads[8]is declared but onlynum_threads = 4threads are used. Consider sizing the array to match actual usage or using a constant.♻️ Suggested fix
- pthread_t threads[8]; - struct thread_data thread_data[8]; + pthread_t threads[4]; + struct thread_data thread_data[4];
1160-1164: Remove unused variable.The
timestampvariable is generated but never used in this test function.♻️ Suggested fix
- time_t now = time(NULL); - struct tm *tm_info = localtime(&now); - char timestamp[32]; - - strftime(timestamp, sizeof(timestamp), "%Y%m%d_%H%M%S", tm_info);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/out_file/file.ctests/runtime/CMakeLists.txttests/runtime/out_file_logrotate.c
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
tests/runtime/CMakeLists.txt
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-08-29T06:25:02.561Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:7-7
Timestamp: 2025-08-29T06:25:02.561Z
Learning: In Fluent Bit, ZSTD (zstandard) compression library is bundled directly in the source tree at `lib/zstd-1.5.7` and is built unconditionally as a static library. Unlike optional external dependencies, ZSTD does not use conditional compilation guards like `FLB_HAVE_ZSTD` and is always available. Headers like `<fluent-bit/flb_zstd.h>` can be included directly without guards.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-08-29T06:24:26.170Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:39-42
Timestamp: 2025-08-29T06:24:26.170Z
Learning: In Fluent Bit, ZSTD compression support is enabled by default and does not require conditional compilation guards (like #ifdef FLB_HAVE_ZSTD) around ZSTD-related code declarations and implementations.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-08-29T06:24:55.855Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:52-56
Timestamp: 2025-08-29T06:24:55.855Z
Learning: ZSTD compression is always available in Fluent Bit and does not require conditional compilation guards. Unlike Arrow/Parquet which use #ifdef FLB_HAVE_ARROW guards, ZSTD is built unconditionally with flb_zstd.c included directly in src/CMakeLists.txt and a bundled ZSTD library at lib/zstd-1.5.7/.
Applied to files:
plugins/out_file/file.c
🧬 Code graph analysis (2)
tests/runtime/out_file_logrotate.c (3)
include/fluent-bit/flb_mem.h (1)
flb_free(126-128)src/flb_lib.c (9)
flb_create(143-225)flb_service_set(652-678)flb_input(266-276)flb_output(279-289)flb_output_set(520-551)flb_start(983-994)flb_lib_push(843-870)flb_stop(1011-1055)flb_destroy(228-263)plugins/in_head/in_head.c (1)
read_bytes(87-106)
plugins/out_file/file.c (6)
include/fluent-bit/flb_mem.h (1)
flb_calloc(84-96)src/flb_lock.c (4)
flb_lock_init(6-17)flb_lock_acquire(32-65)flb_lock_release(67-100)flb_lock_destroy(19-30)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1306-1333)src/flb_pack.c (1)
flb_msgpack_to_json_str(1459-1500)src/flb_sds.c (2)
flb_sds_create(78-90)flb_sds_destroy(389-399)src/flb_log_event_decoder.c (2)
flb_log_event_decoder_init(103-120)flb_log_event_decoder_next(309-456)
🪛 ast-grep (0.40.3)
tests/runtime/out_file_logrotate.c
[warning] 586-586: A check is done with stat and then the file is used. There is no guarantee that the status of the file has not changed since the call to stat which may allow attackers to bypass permission checks.
Context: fopen
Note: [CWE-367]: Time-of-check Time-of-use (TOCTOU) Race Condition [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/FIO45-C.+Avoid+TOCTOU+race+conditions+while+accessing+files
(file-stat-before-action-c)
🔇 Additional comments (20)
tests/runtime/out_file_logrotate.c (9)
1-34: Well-structured test file with comprehensive coverage.Good job setting up cross-platform support with appropriate macros for Windows/POSIX and including the necessary test data. The test function declarations provide clear visibility into the test coverage.
80-203: Robust cross-platform directory cleanup implementation.The helper properly handles both Windows and POSIX platforms, skips special entries, and continues cleanup even on partial failures.
206-248: LGTM!The file counting helper is correctly implemented for both platforms.
254-315: LGTM!The polling helper is well-implemented with proper resource cleanup on each iteration.
318-350: LGTM!The file reading helper properly handles error cases and memory cleanup. The TOCTOU concern (stat before fopen) is acceptable for test code where files are under the test's control.
353-670: Comprehensive format tests with consistent structure.All format tests follow a good pattern: setup, configure, push data, verify content, cleanup. The assertions check format-specific characteristics (CSV commas, LTSV colons, plain JSON, msgpack binary, template substitution).
1358-1403: Good negative test for configuration validation.Testing that
flb_startfails with invalidmax_filesvalues (0 and -1) properly validates the configuration checks in the plugin init.
1405-1483: Excellent edge case coverage for gzip compression.Testing the exact 64KB chunk boundary ensures the streaming gzip implementation handles the edge case where file size equals the chunk size.
1101-1102: Thewait_for_file()function is correctly available. It is defined as a static inline function intests/runtime/flb_tests_runtime.h.in(lines 29-32) and accessible through the#include "flb_tests_runtime.h"header at line 3 of the file. The function call at line 1101 is valid.plugins/out_file/file.c (10)
65-75: Well-designed per-file tracking structure.The
file_file_sizestructure with per-entry locks enables fine-grained concurrent access to different output files.
118-200: Good security measure for tag sanitization.The function properly prevents path traversal attacks by collapsing ".." and "." segments, sanitizing unsafe characters, and handling edge cases like empty tags.
706-880: Solid streaming gzip implementation.The function correctly handles:
- Streaming compression with bounded memory usage
- Proper gzip header/footer with CRC32
- The edge case where file size is exactly a multiple of chunk size (lines 819-843)
- Resource cleanup on all paths
882-969: Well-structured rotation logic with proper cleanup.The function properly handles the rotation workflow: rename, optional gzip compression, cleanup of failed gzip, and size reset. The lock contract is clearly documented.
1155-1173: Bubble sort is adequate for expected file counts.The O(n²) sorting algorithm is acceptable here since
max_filesis typically small (default 7). For very large retention counts, consider using qsort.
1262-1296: Correct hand-over-hand locking pattern.The locking sequence is properly implemented:
- Acquire list_lock
- Find/create entry
- Acquire entry->lock
- Release list_lock
- Perform file operations under entry->lock
- Release entry->lock
This allows concurrent operations on different files while preventing races on the same file.
1562-1570: LGTM!The cleanup section properly frees allocated memory for
directoryandbase_filenameon all exit paths.
1572-1601: Proper cleanup of all resources.The exit handler correctly:
- Acquires list_lock
- Iterates and destroys each entry's lock and filename
- Releases and destroys list_lock
- Frees the context
1639-1654: Good configuration defaults.The rotation feature is disabled by default (
files_rotation = false) preserving backward compatibility. The defaults (100MB max size, 7 files, gzip enabled) are sensible for typical log rotation scenarios.
1658-1668: Plugin configuration is appropriate.With
.workers = 1, the plugin uses a single worker thread. The per-file locking still provides value when the same plugin instance writes to multiple files (based on tag).tests/runtime/CMakeLists.txt (1)
235-238: Correctly integrated into the build system.The new test file is properly added to the Linux-only test block using the existing
FLB_OUT_FILEflag, ensuring it builds alongside the existing out_file tests.
cosmo0920
left a comment
cosmo0920
left a comment
There was a problem hiding this comment.
We need to use 4-spaces width of indentations and also need to add a newline before else keyword.
We need to fix those kind of style errors at first.
SagiROosto
force-pushed
the
out_file-rotation
branch
from
628922c to
fb54c87
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @plugins/out_file/file.c:
- Around line 1249-1262: The flush callback uses plain returns; replace the
plain returns inside the output plugin (e.g., the two occurrences returning
FLB_RETRY after allocating out_file and sanitized_tag) with the
FLB_OUTPUT_RETURN(...) macro so the engine is signaled correctly; update the two
statements that currently read "return FLB_RETRY;" (one after out_file
allocation failure and one after sanitized_tag allocation failure) to use
FLB_OUTPUT_RETURN(FLB_RETRY) and keep existing resource cleanup
(flb_free(out_file), etc.) before invoking the macro.
In @tests/runtime/out_file_logrotate.c:
- Around line 1013-1032: thread_worker contains a typo: it references
*ta->success instead of the correct *data->success; update the error path inside
thread_worker (the block after the flb_lib_push check) to use data->success from
struct thread_data and ensure you lock/unlock data->mutex (already referenced)
correctly so compilation succeeds and the test sets the success flag properly.
- Around line 319-352: In read_file_content replace the typo "*t_size = size;"
with "*out_size = size;" so the function assigns the file size to the provided
out_size parameter (reference: function read_file_content and parameter
out_size); ensure the assignment uses the exact parameter name out_size.
- Around line 587-602: The msgpack verification block contains two unclosed /*
... */ comments causing compilation errors; close the first comment (the
"Msgpack files should not be readable as text" block) by adding "*/" before the
line with fp = fopen(logfile, "rb"); and close the second comment (the "sgpack
typically starts with..." block) by adding "*/" just before the
TEST_CHECK(first_bytes[0] != '{' && first_bytes[0] != '['); line so the fp =
fopen(...) and TEST_CHECK(...) statements are outside the comment blocks.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
build/.emptyplugins/out_file/file.ctests/runtime/CMakeLists.txttests/runtime/out_file_logrotate.c
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/runtime/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-08-29T06:25:02.561Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:7-7
Timestamp: 2025-08-29T06:25:02.561Z
Learning: In Fluent Bit, ZSTD (zstandard) compression library is bundled directly in the source tree at `lib/zstd-1.5.7` and is built unconditionally as a static library. Unlike optional external dependencies, ZSTD does not use conditional compilation guards like `FLB_HAVE_ZSTD` and is always available. Headers like `<fluent-bit/flb_zstd.h>` can be included directly without guards.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-08-29T06:24:26.170Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:39-42
Timestamp: 2025-08-29T06:24:26.170Z
Learning: In Fluent Bit, ZSTD compression support is enabled by default and does not require conditional compilation guards (like #ifdef FLB_HAVE_ZSTD) around ZSTD-related code declarations and implementations.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-08-29T06:24:55.855Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:52-56
Timestamp: 2025-08-29T06:24:55.855Z
Learning: ZSTD compression is always available in Fluent Bit and does not require conditional compilation guards. Unlike Arrow/Parquet which use #ifdef FLB_HAVE_ARROW guards, ZSTD is built unconditionally with flb_zstd.c included directly in src/CMakeLists.txt and a bundled ZSTD library at lib/zstd-1.5.7/.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components, ZSTD support is always available and doesn't need build-time conditionals.
Applied to files:
plugins/out_file/file.c
🧬 Code graph analysis (2)
plugins/out_file/file.c (5)
src/flb_lock.c (4)
flb_lock_init(6-17)flb_lock_acquire(32-65)flb_lock_release(67-100)flb_lock_destroy(19-30)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1306-1333)src/flb_output.c (1)
flb_output_set_context(1602-1605)lib/cmetrics/src/cmt_decode_msgpack.c (1)
cmt_decode_msgpack_create(1450-1496)src/flb_log_event_decoder.c (1)
flb_log_event_decoder_init(103-120)
tests/runtime/out_file_logrotate.c (2)
include/fluent-bit/flb_mem.h (1)
flb_free(126-128)src/flb_lib.c (10)
flb_create(143-225)flb_service_set(652-678)flb_input(266-276)flb_input_set(305-335)flb_output(279-289)flb_output_set(520-551)flb_start(983-994)flb_lib_push(843-870)flb_stop(1011-1055)flb_destroy(228-263)
🪛 Cppcheck (2.19.0)
plugins/out_file/file.c
[error] 497-497: Unmatched '{'. Configuration
(syntaxError)
[error] 497-497: Unmatched '{'. Configuration
(syntaxError)
[error] 497-497: Unmatched '{'. Configuration
(syntaxError)
[error] 497-497: Unmatched '{'. Configuration
(syntaxError)
[error] 497-497: Unmatched '{'. Configuration
(syntaxError)
[error] 497-497: Unmatched '{'. Configuration
(syntaxError)
[error] 497-497: Unmatched '{'. Configuration
(syntaxError)
[error] 497-497: Unmatched '{'. Configuration
(syntaxError)
[error] 497-497: Unmatched '{'. Configuration
(syntaxError)
[error] 497-497: Unmatched '{'. Configuration
(syntaxError)
[error] 497-497: Unmatched '{'. Configuration
(syntaxError)
[error] 497-497: Unmatched '{'. Configuration
(syntaxError)
tests/runtime/out_file_logrotate.c
[error] 538-538: Unmatched '{'. Configuration
(syntaxError)
[error] 538-538: Unmatched '{'. Configuration
(syntaxError)
[error] 538-538: Unmatched '{'. Configuration
(syntaxError)
[error] 538-538: Unmatched '{'. Configuration
(syntaxError)
[error] 538-538: Unmatched '{'. Configuration
(syntaxError)
[error] 538-538: Unmatched '{'. Configuration
(syntaxError)
[error] 538-538: Unmatched '{'. Configuration
(syntaxError)
[error] 538-538: Unmatched '{'. Configuration
(syntaxError)
[error] 538-538: Unmatched '{'. Configuration
(syntaxError)
[error] 538-538: Unmatched '{'. Configuration
(syntaxError)
[error] 538-538: Unmatched '{'. Configuration
(syntaxError)
[error] 538-538: Unmatched '{'. Configuration
(syntaxError)
🔇 Additional comments (1)
plugins/out_file/file.c (1)
1343-1350: Memory leak ofdirectoryon allocation failure ofbase_filename.If
base_filenameallocation fails at line 1344, the code releases the entry lock and jumps to cleanup, butdirectorywas already allocated at line 1333 and will be freed in cleanup at line 1596-1598. Wait, let me re-check...Actually, looking at the cleanup section (lines 1595-1607),
directoryandbase_filenameare freed if non-NULL. So this is handled correctly since the goto goes to cleanup.
SagiROosto
force-pushed
the
out_file-rotation
branch
from
fb54c87 to
637cd27
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @tests/runtime/out_file_logrotate.c:
- Around line 329-362: The read_file_content function has a TOCTOU between
stat() and fopen(); fix it by opening the file first with open() to obtain a
file descriptor, call fstat() on that fd to get the size, then use fdopen() (or
read from the fd directly) to read the contents; ensure you replace
stat(filename, &st) and fopen(filename, "rb") with this open->fstat->fdopen
sequence, handle errors by closing the fd and freeing buffers on failure, and
set *out_size and return the allocated buffer as before.
🧹 Nitpick comments (1)
plugins/out_file/file.c (1)
1463-1468: Consider making fsync optional for performance.The code uses
fsync()after every flush to ensure data is written to disk before updating the size counter (lines 1463-1468, 1510-1515, 1631-1636). While this ensures data durability, it can significantly impact performance in high-throughput scenarios.Since the size counter is used only for rotation decisions and not for data integrity, consider:
- Making fsync optional via a configuration parameter (e.g.,
fsync_on_flush)- Using
fflush()alone for size tracking (already ensures buffered data is written)- Accepting that size tracking may be slightly behind actual file size in some cases
The current approach prioritizes correctness over performance, which is reasonable for the default behavior.
Also applies to: 1510-1515, 1631-1636
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/out_file/file.ctests/runtime/CMakeLists.txttests/runtime/out_file_logrotate.c
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/runtime/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-29T06:25:02.561Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:7-7
Timestamp: 2025-08-29T06:25:02.561Z
Learning: In Fluent Bit, ZSTD (zstandard) compression library is bundled directly in the source tree at `lib/zstd-1.5.7` and is built unconditionally as a static library. Unlike optional external dependencies, ZSTD does not use conditional compilation guards like `FLB_HAVE_ZSTD` and is always available. Headers like `<fluent-bit/flb_zstd.h>` can be included directly without guards.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-08-29T06:24:26.170Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:39-42
Timestamp: 2025-08-29T06:24:26.170Z
Learning: In Fluent Bit, ZSTD compression support is enabled by default and does not require conditional compilation guards (like #ifdef FLB_HAVE_ZSTD) around ZSTD-related code declarations and implementations.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-08-29T06:24:55.855Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:52-56
Timestamp: 2025-08-29T06:24:55.855Z
Learning: ZSTD compression is always available in Fluent Bit and does not require conditional compilation guards. Unlike Arrow/Parquet which use #ifdef FLB_HAVE_ARROW guards, ZSTD is built unconditionally with flb_zstd.c included directly in src/CMakeLists.txt and a bundled ZSTD library at lib/zstd-1.5.7/.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components, ZSTD support is always available and doesn't need build-time conditionals.
Applied to files:
plugins/out_file/file.c
🧬 Code graph analysis (2)
plugins/out_file/file.c (5)
src/flb_lock.c (4)
flb_lock_init(6-17)flb_lock_acquire(32-65)flb_lock_release(67-100)flb_lock_destroy(19-30)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1306-1333)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)src/flb_sds.c (1)
flb_sds_create(78-90)src/flb_log_event_decoder.c (1)
flb_log_event_decoder_destroy(151-178)
tests/runtime/out_file_logrotate.c (2)
include/fluent-bit/flb_mem.h (1)
flb_free(126-128)src/flb_lib.c (6)
flb_create(143-225)flb_service_set(652-678)flb_input(266-276)flb_lib_push(843-870)flb_stop(1011-1055)flb_destroy(228-263)
🪛 ast-grep (0.40.4)
tests/runtime/out_file_logrotate.c
[warning] 603-603: A check is done with stat and then the file is used. There is no guarantee that the status of the file has not changed since the call to stat which may allow attackers to bypass permission checks.
Context: fopen
Note: [CWE-367]: Time-of-check Time-of-use (TOCTOU) Race Condition [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/FIO45-C.+Avoid+TOCTOU+race+conditions+while+accessing+files
(file-stat-before-action-c)
🔇 Additional comments (11)
plugins/out_file/file.c (7)
228-230: LGTM! Proper initialization of rotation structures.The rotation subsystem is correctly initialized with both the file_sizes list and the list_lock mutex, establishing the foundation for thread-safe per-file tracking.
238-244: Good validation: max_files must be positive when rotation is enabled.This validation prevents misconfigurations that could lead to unexpected behavior. The error message is clear and actionable.
634-691: Well-structured entry management with proper error handling.Both helper functions follow best practices:
- Caller holds
list_lock(documented clearly)create_file_size_entryhas complete error handling with cleanup- Lock initialization failure is properly checked
- Dynamic string (
flb_sds_t) is used correctly
851-882: Excellent edge-case handling for exact chunk-size files.This code correctly handles the scenario where a file's size is an exact multiple of
GZIP_CHUNK_SIZE. Whenfreadreturns 0, the loop exits without callingdeflatewithZ_FINISH, so this block ensures the stream is properly finalized.
1199-1217: Bubble sort is acceptable for typical rotation file counts.While O(n²) complexity is generally suboptimal, it's reasonable here because:
- File counts are typically small (max_files defaults to 7)
- Sorting overhead is negligible compared to I/O operations
- Code simplicity is prioritized over micro-optimization
1314-1409: Excellent hand-over-hand locking pattern for thread safety.The implementation correctly:
- Acquires
list_lockto search/create entry- Acquires
entry->lockbefore any file operations- Releases
list_lock(minimizing contention)- Performs rotation logic while holding
entry->lock- Properly releases locks on all error paths
This design allows multiple threads to work on different files concurrently while protecting per-file operations.
740-919: The streaming gzip compression implementation correctly uses the miniz API with proper parameters and error handling. ThedeflateInit2()call withZ_DEFAULT_COMPRESSION, Z_DEFLATED, -Z_DEFAULT_WINDOW_BITS, 9, Z_DEFAULT_STRATEGYmatches the established pattern insrc/flb_gzip.c. The buffer sizing withmz_compressBound(), streaming with appropriate flush modes (Z_NO_FLUSHandZ_FINISH), and edge case handling for files that are exact multiples of the chunk size (lines 851-882) are all implemented correctly.tests/runtime/out_file_logrotate.c (4)
365-425: Format tests follow good testing patterns.The CSV format test correctly:
- Sets up isolated test environment
- Configures rotation with format-specific options
- Writes sample data and allows flush time
- Validates format-specific characteristics (commas, timestamp)
- Cleans up test artifacts
This pattern is consistently applied across all format tests.
1444-1523: Excellent edge-case test for exact chunk-size compression.This test specifically exercises the code path at lines 851-882 in
file.c, where a file's size is an exact multiple ofGZIP_CHUNK_SIZE(64KB). This ensures the streaming compression correctly finalizes the stream even when the lastfreadreturns 0.The test:
- Creates exactly 64KB of data
- Triggers rotation
- Verifies gzipped output exists
This validates the fix for a subtle bug that could leave compressed files incomplete.
1058-1177: Multithreaded test validates concurrent rotation safety.This test effectively validates the thread-safety of the rotation implementation by:
- Creating 4 concurrent threads writing to the same output
- Using shared mutex to protect the success flag
- Verifying all data was written correctly
- Checking file content integrity
The test correctly initializes the mutex with
PTHREAD_MUTEX_INITIALIZER(line 1069) and destroys it withpthread_mutex_destroy(line 1173).One minor note: The mutex is used only for the
successflag. The actual thread-safety of the file plugin is tested implicitly through concurrentflb_lib_pushcalls, which exercise the hand-over-hand locking incb_file_flush.
36-72: Comprehensive test suite covering all rotation features.The test list includes 16 tests that thoroughly validate:
- Core rotation functionality (basic, gzip, max_files)
- All output formats (csv, ltsv, plain, msgpack, template)
- Configuration options (path, mkdir, delimiters, column names)
- Edge cases (exact chunk size, invalid config)
- Concurrent access (multithreaded)
This provides excellent coverage of the new rotation feature across multiple dimensions.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 637cd2703e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
SagiROosto
force-pushed
the
out_file-rotation
branch
from
637cd27 to
e6bb50d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
plugins/out_file/file.c (2)
1147-1154: Handleflb_strdupfailure to avoid NULL entries in files array.If
flb_strdupfails (returns NULL), the indexiis still incremented butfiles[i]will be NULL. This is handled defensively in the sort and delete loops, but for correctness it would be better to only incremention successful allocation.Suggested fix
if (is_valid_rotation_filename(findData.cFileName, pattern) && i < file_count) { snprintf(full_path, PATH_MAX - 1, "%s" FLB_PATH_SEPARATOR "%s", directory, findData.cFileName); - files[i] = flb_strdup(full_path); - i++; + files[i] = flb_strdup(full_path); + if (files[i] != NULL) { + i++; + } }Apply similar fix for the POSIX path at lines 1189-1193.
1476-1482: Consider usingfdatasyncinstead offsyncfor better performance.Since you only need accurate file size tracking (data, not all metadata),
fdatasync()would be sufficient and slightly faster thanfsync()as it doesn't force sync of metadata that doesn't affect data retrieval (like access time).Suggested change
#ifndef FLB_SYSTEM_WINDOWS /* Only fsync when rotation is enabled - needed for accurate size tracking */ if (ctx->files_rotation == FLB_TRUE && ctx->max_size > 0) { - fsync(fileno(fp)); + fdatasync(fileno(fp)); } #endifApply to all three
fsynccall sites (lines 1480, 1529, 1652).tests/runtime/out_file_logrotate.c (2)
1190-1194: Remove unused timestamp generation code.The
tm_info,timestampvariable andstrftimecall are not used anywhere in this test function. This appears to be leftover debug code.Suggested fix
int out_ffd; FILE *fp; char logfile[512]; - time_t now = time(NULL); - struct tm *tm_info = localtime(&now); - char timestamp[32]; - - strftime(timestamp, sizeof(timestamp), "%Y%m%d_%H%M%S", tm_info); /* Clean up any existing directory and contents */
1177-1177: Remove extraneous semicolons after function definitions.There are semicolons after the closing braces of
flb_test_file_logrotate_multithreaded(line 1177),flb_test_file_logrotate_basic_rotation(line 1261), andflb_test_file_logrotate_gzip_compression(line 1328). While harmless, they are non-standard.Suggested fix
-}; +}Apply to lines 1177, 1261, and 1328.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/out_file/file.ctests/runtime/CMakeLists.txttests/runtime/out_file_logrotate.c
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/runtime/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-08-29T06:25:02.561Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:7-7
Timestamp: 2025-08-29T06:25:02.561Z
Learning: In Fluent Bit, ZSTD (zstandard) compression library is bundled directly in the source tree at `lib/zstd-1.5.7` and is built unconditionally as a static library. Unlike optional external dependencies, ZSTD does not use conditional compilation guards like `FLB_HAVE_ZSTD` and is always available. Headers like `<fluent-bit/flb_zstd.h>` can be included directly without guards.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-08-29T06:24:26.170Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:39-42
Timestamp: 2025-08-29T06:24:26.170Z
Learning: In Fluent Bit, ZSTD compression support is enabled by default and does not require conditional compilation guards (like #ifdef FLB_HAVE_ZSTD) around ZSTD-related code declarations and implementations.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-08-29T06:24:55.855Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:52-56
Timestamp: 2025-08-29T06:24:55.855Z
Learning: ZSTD compression is always available in Fluent Bit and does not require conditional compilation guards. Unlike Arrow/Parquet which use #ifdef FLB_HAVE_ARROW guards, ZSTD is built unconditionally with flb_zstd.c included directly in src/CMakeLists.txt and a bundled ZSTD library at lib/zstd-1.5.7/.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components, ZSTD support is always available and doesn't need build-time conditionals.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
tests/runtime/out_file_logrotate.c
🧬 Code graph analysis (1)
plugins/out_file/file.c (4)
src/flb_lock.c (4)
flb_lock_init(6-17)flb_lock_acquire(32-65)flb_lock_release(67-100)flb_lock_destroy(19-30)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1306-1333)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)src/flb_sds.c (2)
flb_sds_create(78-90)flb_sds_destroy(389-399)
🪛 ast-grep (0.40.4)
tests/runtime/out_file_logrotate.c
[warning] 603-603: A check is done with stat and then the file is used. There is no guarantee that the status of the file has not changed since the call to stat which may allow attackers to bypass permission checks.
Context: fopen
Note: [CWE-367]: Time-of-check Time-of-use (TOCTOU) Race Condition [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/FIO45-C.+Avoid+TOCTOU+race+conditions+while+accessing+files
(file-stat-before-action-c)
🔇 Additional comments (8)
plugins/out_file/file.c (3)
1692-1708: LGTM - Proper cleanup of rotation state.The exit callback correctly acquires the list lock, safely iterates and cleans up all file size entries (destroying per-entry locks and freeing memory), then releases and destroys the list lock. This is the correct order of operations.
1768-1790: Good default configuration for rotation feature.The rotation feature is disabled by default (
files_rotation = false), preserving backward compatibility. The default values (100MB max_size, 7 max_files, gzip enabled) are sensible for typical use cases.
238-244: LGTM - Proper validation of rotation configuration.The validation ensures
max_files >= 1when rotation is enabled, with clear error messaging. Memory cleanup is handled correctly on validation failure.tests/runtime/out_file_logrotate.c (5)
329-362: TOCTOU race condition inread_file_contentis acceptable for tests.The static analyzer flagged a time-of-check-time-of-use (TOCTOU) race between
stat()andfopen(). In production code this could be exploited, but for test helper functions operating on controlled test directories, this is acceptable. The test files are created/controlled by the test itself.
54-72: Comprehensive test coverage for the rotation feature.The TEST_LIST covers all major scenarios: basic rotation, gzip compression, max_files enforcement, various output formats (csv, ltsv, plain, msgpack, template), configuration options (path, mkdir, delimiter), and multithreaded scenarios. This aligns well with the PR objectives from linked issue #11110.
1179-1261: LGTM - Basic rotation test is well-structured.The test properly verifies that:
- Writing data beyond
max_sizetriggers rotation- The original file continues to exist after rotation
- At least one rotated file is created
Good use of
count_files_in_directoryto verify rotation occurred.
1396-1442: Good negative testing for configuration validation.This test verifies that invalid
max_filesvalues (0 and -1) cause plugin initialization to fail, ensuring the validation logic incb_file_initworks correctly.
709-713: Pragma usage is acceptable for test code.The GCC diagnostic pragmas suppress format-truncation warnings for
snprintfcalls where the paths are known to be short. The comments document the rationale. This is reasonable for test code with controlled inputs.
plugins/out_file/file.c
Outdated
| #ifdef FLB_SYSTEM_WINDOWS | ||
| localtime_s(&tm_info, &now); | ||
| #else | ||
| localtime_r(&now, &tm_info); | ||
| #endif |
There was a problem hiding this comment.
We don't have to distinguish localtime_r because we already have the equivalent implementation of this macro switch in:
https://github.com/fluent/fluent-bit/blob/master/include/fluent-bit/flb_compat.h#L97
So, we can just use localtime_r in plugins' implementations.
SagiROosto
force-pushed
the
out_file-rotation
branch
from
e6bb50d to
2a404ed
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/out_file/file.c (1)
168-205:sanitize_tag_name()path safety looks good; consider handling both separators on Windows.Allowing only
[A-Za-z0-9._-]and collapsing./..components is solid for preventing traversal. One edge case: on Windows you later search forFLB_PATH_SEPARATOR[0]when extracting the basename; if a user-provided path contains/, basename detection may miss it (minor).
🤖 Fix all issues with AI agents
In `@plugins/out_file/file.c`:
- Around line 1320-1427: Rotation failures are currently ignored: when
rotate_file(ctx, out_file, entry) fails the code continues to append and can
silently grow files past ctx->max_size; change the flow so a rotation failure
returns an error (e.g., FLB_RETRY or FLB_ERROR) to the caller or at minimum logs
a persistent error per file. Specifically, inside the block where file_size >=
ctx->max_size, after calling rotate_file(ctx, out_file, entry) check for
non-zero return and on failure release entry->lock and return FLB_RETRY (or
FLB_ERROR per desired contract), or add a flb_plg_error with file-specific state
and set a flag to suppress repeated logs; ensure you reference rotate_file,
entry->lock, file_size and ctx->max_size when making the change so the caller
observes rotation failure instead of silently proceeding.
In `@tests/runtime/out_file_logrotate.c`:
- Around line 1365-1391: The comment saying "Write ~5MB" is incorrect for the
loop that calls flb_lib_push 5 * 4 * JSON_SMALL times (closer to ~80KB if
JSON_SMALL ≈ 4KB); update the comment above the outer for-loop (around the
flb_lib_push calls) to reflect the actual approximate total written (or state it
writes multiple chunks to trigger rotations) and/or adjust the loop counts if
you really want ~5MB; reference the flb_lib_push loop, JSON_SMALL, and the
surrounding flb_time_msleep/count_files_in_directory checks when making the
change.
- Around line 1263-1329: The function flb_test_file_logrotate_gzip_compression
has an extra semicolon after its closing brace ("};") which causes a compile
error; remove the trailing semicolon so the function ends with just "}" (i.e.,
replace "};" with "}") to fix the syntax error in the definition of
flb_test_file_logrotate_gzip_compression.
- Around line 1179-1262: The test function
flb_test_file_logrotate_basic_rotation contains a stray closing brace+semicolon
("};") and unused local variables timestamp and tm_info that trigger -Werror;
fix by removing the extraneous semicolon so the function ends with "}" only and
delete the unused variables timestamp and tm_info (and any related strftime
call) from flb_test_file_logrotate_basic_rotation so no unused-local warnings
remain.
- Around line 1058-1177: The function flb_test_file_logrotate_multithreaded ends
with an extra semicolon after its closing brace ('};') which is a syntax error;
remove the trailing semicolon so the function ends with just '}' (i.e., change
'};' to '}'), then recompile/run the tests to ensure the file now builds and the
test runs as expected.
- Around line 1444-1523: The snprintf call in
flb_test_file_logrotate_gzip_compression_exact_chunk uses "%lu" with time(NULL),
which is non-portable for time_t; change the format to use "%lld" and cast the
time value to (long long) (e.g., snprintf(json_payload, json_size, "[%lld,
{\"message\": \"%s\"}]", (long long)time(NULL), large_message)) so the timestamp
is formatted portably; update the snprintf invocation that constructs
json_payload accordingly.
♻️ Duplicate comments (3)
plugins/out_file/file.c (2)
228-244: Init error paths should destroyctx->list_lock(and checkflb_lock_init)
flb_lock_init(&ctx->list_lock);isn’t checked, and on later failures (flb_output_config_map_setfailure / invalidmax_files) the lock isn’t destroyed before freeingctx.Proposed fix
/* Initialize rotation-related structures */ mk_list_init(&ctx->file_sizes); - flb_lock_init(&ctx->list_lock); + if (flb_lock_init(&ctx->list_lock) != 0) { + flb_plg_error(ins, "failed to initialize list lock"); + flb_free(ctx); + return -1; + } ret = flb_output_config_map_set(ins, (void *)ctx); if (ret == -1) { + flb_lock_destroy(&ctx->list_lock); flb_free(ctx); return -1; } /* Validate max_files only if rotation is enabled */ if (ctx->files_rotation == FLB_TRUE && ctx->max_files <= 0) { flb_plg_error(ctx->ins, "invalid max_files=%d; must be >= 1", ctx->max_files); + flb_lock_destroy(&ctx->list_lock); flb_free(ctx); return -1; }
916-1007: Rotated filename can collide within the same second; also fixsnprintfbuffer sizes.Timestamp format is seconds-only (
%Y%m%d_%H%M%S), so multiple rotations in one second can collide. Also,snprintf(..., PATH_MAX - 1, ...)needlessly reduces capacity and can risk missing a terminator when the code later assumes a fullPATH_MAXbuffer.Proposed direction
- snprintf(rotated_filename, PATH_MAX - 1, "%s.%s", filename, timestamp); + snprintf(rotated_filename, PATH_MAX, "%s.%s", filename, timestamp); ... - snprintf(gzip_filename, PATH_MAX - 1, "%s.gz", rotated_filename); + snprintf(gzip_filename, PATH_MAX, "%s.gz", rotated_filename);(Separately: add sub-second precision or a monotonic counter to the suffix.)
tests/runtime/out_file_logrotate.c (1)
328-362: Avoidstat()→fopen()TOCTOU inread_file_content()(even in tests).This is test-only, but it’s easy to eliminate by
fopen()first and thenfstat(fileno(fp))for sizing.
🧹 Nitpick comments (5)
tests/runtime/CMakeLists.txt (1)
234-238: Add logrotate runtime test is fine; consider simplifyingif (${BUILT})usage.The new
out_file_logrotate.cruntime test being Linux-only looks right underif(NOT FLB_SYSTEM_WINDOWS). If you ever pass non-boolean-ish values intoFLB_RT_TEST,if (${BUILT})can expand strangely;if(BUILT)(orif(${BUILT} STREQUAL "ON")style) is typically more robust in CMake macros.plugins/out_file/file.c (3)
688-699: Preferlocaltime_r()directly (compat already handled).The repo already provides a
localtime_rcompatibility shim, so the#ifdef FLB_SYSTEM_WINDOWSsplit can likely be removed andlocaltime_r()used consistently. Based on learnings, this is the preferred style.
735-914: Gzip streaming implementation is generally solid; verify miniz zlib-compat expectations and tighten write error handling.A couple of low-cost hardening points:
- Check
fwrite()results for header/footer too (not just compressed blocks).- Consider logging
statusfromdeflateInit2/deflatefor easier triage.
1066-1244:cleanup_old_files()is OK for smallmax_files; consider lexicographic sort to avoidstat()storm.Because rotated names embed sortable timestamps, you can sort by filename (or parse the timestamp) instead of
stat()ing every pair (current O(n²) + n stats per comparison). Probably fine given typicalmax_files, but easy to optimize if you expect large directories.tests/runtime/out_file_logrotate.c (1)
1127-1169: Multithreaded assertions are good; line counting viafgets(4096)may inflate counts on long lines.If lines can exceed 4096,
fgetswill split one record into multiple reads, inflatingline_count. Consider counting record delimiters (\n) in the full file buffer instead.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/out_file/file.ctests/runtime/CMakeLists.txttests/runtime/out_file_logrotate.c
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
tests/runtime/CMakeLists.txt
📚 Learning: 2025-09-04T12:32:46.030Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10825
File: plugins/out_s3/s3.c:0-0
Timestamp: 2025-09-04T12:32:46.030Z
Learning: In Fluent Bit plugins, avoid creating plugin-local config_map entries that duplicate core functionality. The core already provides Retry_Limit parsed into ins->retry_limit, so plugins should use that directly rather than defining their own retry_limit configuration option.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-08-29T06:25:02.561Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:7-7
Timestamp: 2025-08-29T06:25:02.561Z
Learning: In Fluent Bit, ZSTD (zstandard) compression library is bundled directly in the source tree at `lib/zstd-1.5.7` and is built unconditionally as a static library. Unlike optional external dependencies, ZSTD does not use conditional compilation guards like `FLB_HAVE_ZSTD` and is always available. Headers like `<fluent-bit/flb_zstd.h>` can be included directly without guards.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-08-29T06:24:26.170Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:39-42
Timestamp: 2025-08-29T06:24:26.170Z
Learning: In Fluent Bit, ZSTD compression support is enabled by default and does not require conditional compilation guards (like `#ifdef` FLB_HAVE_ZSTD) around ZSTD-related code declarations and implementations.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-08-29T06:24:55.855Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:52-56
Timestamp: 2025-08-29T06:24:55.855Z
Learning: ZSTD compression is always available in Fluent Bit and does not require conditional compilation guards. Unlike Arrow/Parquet which use `#ifdef` FLB_HAVE_ARROW guards, ZSTD is built unconditionally with flb_zstd.c included directly in src/CMakeLists.txt and a bundled ZSTD library at lib/zstd-1.5.7/.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components, ZSTD support is always available and doesn't need build-time conditionals.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
tests/runtime/out_file_logrotate.c
🧬 Code graph analysis (1)
plugins/out_file/file.c (3)
src/flb_lock.c (4)
flb_lock_init(6-17)flb_lock_acquire(32-65)flb_lock_release(67-100)flb_lock_destroy(19-30)src/flb_output.c (2)
flb_output_get_property(1125-1128)flb_output_set_context(1602-1605)src/flb_log_event_decoder.c (2)
flb_log_event_decoder_init(103-120)flb_log_event_decoder_next(309-456)
🪛 ast-grep (0.40.5)
tests/runtime/out_file_logrotate.c
[warning] 603-603: A check is done with stat and then the file is used. There is no guarantee that the status of the file has not changed since the call to stat which may allow attackers to bypass permission checks.
Context: fopen
Note: [CWE-367]: Time-of-check Time-of-use (TOCTOU) Race Condition [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/FIO45-C.+Avoid+TOCTOU+race+conditions+while+accessing+files
(file-stat-before-action-c)
🔇 Additional comments (3)
plugins/out_file/file.c (2)
65-95: Rotation state additions look reasonable (list + per-file lock).The
struct file_file_size+ctx->file_sizes+ctx->list_lockdesign is a clear way to keep per-target-file rotation state without blocking unrelated files.
1482-1658: Gatingfsync()behind rotation is the right tradeoff.The
fflush()+ conditionalfsync()only when rotation is enabled avoids the worst throughput penalty in the non-rotation default path.tests/runtime/out_file_logrotate.c (1)
1-77: Test registration + cross-platform scaffolding is clean.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
SagiROosto
force-pushed
the
out_file-rotation
branch
from
2a404ed to
2582108
Compare
Signed-off-by: SagiROosto <[email protected]>
SagiROosto
force-pushed
the
out_file-rotation
branch
from
2582108 to
9573977
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/runtime/out_file_logrotate.c`:
- Line 1129: The call to wait_for_file(...) is invalid because only
wait_for_file_pattern(...) exists; replace the call to wait_for_file(logfile,
1000, TEST_TIMEOUT) with an appropriate call to wait_for_file_pattern(logfile,
/*min_size=*/1000, TEST_TIMEOUT) or implement a small helper function named
wait_for_file(const char *path, size_t min_size, int timeout_ms) that forwards
to wait_for_file_pattern(path, min_size, timeout_ms); ensure the argument
semantics match existing helpers like wait_for_file_pattern and update any
callers accordingly.
♻️ Duplicate comments (7)
tests/runtime/out_file_logrotate.c (5)
1495-1496: Use portable format specifier fortime_t.Using
%luwithtime(NULL)is non-portable sincetime_tmay not beunsigned longon all platforms (it could belong,long long, or even signed).🔧 Proposed fix
- snprintf(json_payload, json_size, "[%lu, {\"message\": \"%s\"}]", - time(NULL), large_message); + snprintf(json_payload, json_size, "[%lld, {\"message\": \"%s\"}]", + (long long)time(NULL), large_message);
1330-1330: Remove trailing semicolon after function definition.The function
flb_test_file_logrotate_gzip_compression()ends with};which is invalid C syntax.🐛 Proposed fix
/* Clean up directory and all contents */ recursive_delete_directory(TEST_LOGPATH); -}; +}
1178-1178: Remove trailing semicolon after function definition.The function
flb_test_file_logrotate_multithreaded()ends with};which is invalid C syntax after a function definition.🐛 Proposed fix
/* Clean up directory and all contents */ recursive_delete_directory(TEST_LOGPATH); -}; +}
1263-1263: Remove trailing semicolon after function definition.The function
flb_test_file_logrotate_basic_rotation()ends with};which is invalid C syntax.🐛 Proposed fix
/* Clean up directory and all contents */ recursive_delete_directory(TEST_LOGPATH); -}; +}
1191-1196: Remove unused timestamp variables to avoid compiler warnings.The
timestamp,tm_info, and related code generate a timestamp string that is never used in the test. This will trigger-Wunused-variablewarnings with-Werror.🔧 Proposed fix
FILE *fp; char logfile[512]; - time_t now = time(NULL); - struct tm tm_info; - char timestamp[32]; - - localtime_r(&now, &tm_info); - strftime(timestamp, sizeof(timestamp), "%Y%m%d_%H%M%S", &tm_info); /* Clean up any existing directory and contents */plugins/out_file/file.c (2)
229-245: Missing lock cleanup on error paths.If
flb_lock_initsucceeds butflb_output_config_map_setormax_filesvalidation fails, thelist_lockis not destroyed before freeingctx. Also, the return value offlb_lock_initis not checked.🔧 Proposed fix
/* Initialize rotation-related structures */ mk_list_init(&ctx->file_sizes); - flb_lock_init(&ctx->list_lock); + if (flb_lock_init(&ctx->list_lock) != 0) { + flb_plg_error(ins, "failed to initialize list lock"); + flb_free(ctx); + return -1; + } ret = flb_output_config_map_set(ins, (void *)ctx); if (ret == -1) { + flb_lock_destroy(&ctx->list_lock); flb_free(ctx); return -1; } /* Validate max_files only if rotation is enabled */ if (ctx->files_rotation == FLB_TRUE && ctx->max_files <= 0) { flb_plg_error(ctx->ins, "invalid max_files=%d; must be >= 1", ctx->max_files); + flb_lock_destroy(&ctx->list_lock); flb_free(ctx); return -1; }
689-696: Rotation timestamp lacks sub-second precision - risk of filename collisions.The timestamp format
%Y%m%d_%H%M%Shas only second-level granularity. Ifmax_sizeis small and output rate is high, multiple rotations within the same second will produce identical filenames, potentially overwriting previous rotations or causingrename()failures.Consider adding sub-second precision or a monotonic counter.
🔧 Proposed fix using milliseconds
static void generate_timestamp(char *timestamp, size_t size) { - time_t now = time(NULL); - struct tm tm_info; - localtime_r(&now, &tm_info); - strftime(timestamp, size, "%Y%m%d_%H%M%S", &tm_info); + struct timespec ts; + struct tm tm_info; + clock_gettime(CLOCK_REALTIME, &ts); + localtime_r(&ts.tv_sec, &tm_info); + snprintf(timestamp, size, "%04d%02d%02d_%02d%02d%02d_%03ld", + tm_info.tm_year + 1900, tm_info.tm_mon + 1, tm_info.tm_mday, + tm_info.tm_hour, tm_info.tm_min, tm_info.tm_sec, + ts.tv_nsec / 1000000); }Note: Update
is_valid_rotation_filename()to accept the new format length accordingly.
🧹 Nitpick comments (2)
plugins/out_file/file.c (2)
1191-1209: Consider more efficient sorting for large file counts.The bubble sort (O(n²)) is acceptable for typical
max_filesvalues but could be slow if many rotated files exist. For most use cases this is fine, but ifmax_filesis large, consider using qsort().
1700-1701: Consider logging if lock acquisition fails during exit.The return value of
flb_lock_acquire()is not checked. While this is during shutdown and unlikely to cause issues, logging a warning would help debugging if cleanup fails.🔧 Proposed fix
/* Free all file size entries from linked list */ - flb_lock_acquire(&ctx->list_lock, FLB_LOCK_DEFAULT_RETRY_LIMIT, - FLB_LOCK_DEFAULT_RETRY_DELAY); + if (flb_lock_acquire(&ctx->list_lock, FLB_LOCK_DEFAULT_RETRY_LIMIT, + FLB_LOCK_DEFAULT_RETRY_DELAY) != 0) { + flb_plg_warn(ctx->ins, "failed to acquire list lock during exit, proceeding with cleanup"); + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/out_file/file.ctests/runtime/CMakeLists.txttests/runtime/out_file_logrotate.c
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
tests/runtime/CMakeLists.txt
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
tests/runtime/out_file_logrotate.c
📚 Learning: 2025-09-04T12:32:46.030Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10825
File: plugins/out_s3/s3.c:0-0
Timestamp: 2025-09-04T12:32:46.030Z
Learning: In Fluent Bit plugins, avoid creating plugin-local config_map entries that duplicate core functionality. The core already provides Retry_Limit parsed into ins->retry_limit, so plugins should use that directly rather than defining their own retry_limit configuration option.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-08-29T06:25:02.561Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:7-7
Timestamp: 2025-08-29T06:25:02.561Z
Learning: In Fluent Bit, ZSTD (zstandard) compression library is bundled directly in the source tree at `lib/zstd-1.5.7` and is built unconditionally as a static library. Unlike optional external dependencies, ZSTD does not use conditional compilation guards like `FLB_HAVE_ZSTD` and is always available. Headers like `<fluent-bit/flb_zstd.h>` can be included directly without guards.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-09-08T11:21:33.975Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 10851
File: include/fluent-bit/flb_simd.h:60-66
Timestamp: 2025-09-08T11:21:33.975Z
Learning: Fluent Bit currently only supports MSVC compiler on Windows, so additional compiler compatibility guards may be unnecessary for Windows-specific code paths.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-08-29T06:24:26.170Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:39-42
Timestamp: 2025-08-29T06:24:26.170Z
Learning: In Fluent Bit, ZSTD compression support is enabled by default and does not require conditional compilation guards (like `#ifdef` FLB_HAVE_ZSTD) around ZSTD-related code declarations and implementations.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-08-29T06:24:55.855Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:52-56
Timestamp: 2025-08-29T06:24:55.855Z
Learning: ZSTD compression is always available in Fluent Bit and does not require conditional compilation guards. Unlike Arrow/Parquet which use `#ifdef` FLB_HAVE_ARROW guards, ZSTD is built unconditionally with flb_zstd.c included directly in src/CMakeLists.txt and a bundled ZSTD library at lib/zstd-1.5.7/.
Applied to files:
plugins/out_file/file.c
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components, ZSTD support is always available and doesn't need build-time conditionals.
Applied to files:
plugins/out_file/file.c
🧬 Code graph analysis (2)
tests/runtime/out_file_logrotate.c (3)
include/fluent-bit/flb_mem.h (1)
flb_free(126-128)src/flb_lib.c (9)
flb_create(143-225)flb_service_set(652-678)flb_input(266-276)flb_output(279-289)flb_output_set(520-551)flb_start(983-994)flb_lib_push(843-870)flb_stop(1011-1055)flb_destroy(228-263)plugins/in_head/in_head.c (1)
read_bytes(87-106)
plugins/out_file/file.c (1)
src/flb_lock.c (1)
flb_lock_init(6-17)
🪛 ast-grep (0.40.5)
tests/runtime/out_file_logrotate.c
[warning] 604-604: A check is done with stat and then the file is used. There is no guarantee that the status of the file has not changed since the call to stat which may allow attackers to bypass permission checks.
Context: fopen
Note: [CWE-367]: Time-of-check Time-of-use (TOCTOU) Race Condition [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/FIO45-C.+Avoid+TOCTOU+race+conditions+while+accessing+files
(file-stat-before-action-c)
🔇 Additional comments (7)
tests/runtime/out_file_logrotate.c (1)
54-73: Good comprehensive test coverage.The test suite covers a wide range of rotation scenarios including basic rotation, gzip compression, max_files cleanup, multiple output formats, path/mkdir options, delimiters, and multithreaded scenarios. This aligns well with the PR objectives.
plugins/out_file/file.c (5)
732-911: Well-implemented streaming gzip compression.The streaming gzip implementation correctly handles:
- Memory-safe chunked processing avoiding large allocations
- Edge case where file size is a multiple of
GZIP_CHUNK_SIZE- Proper CRC32 calculation and gzip footer
- Comprehensive error handling with cleanup paths
913-1004: Rotation logic is sound with proper error handling.The function correctly:
- Documents locking requirements
- Handles both gzip-enabled and gzip-disabled cases
- Cleans up failed gzip files on error
- Preserves the renamed file on compression failure (no data loss)
1317-1424: Rotation and locking logic is well-implemented.The hand-over-hand locking pattern is correctly implemented:
- List lock protects entry lookup/creation
- Entry lock protects file operations
- List lock is released before file I/O to avoid contention
- All error paths correctly release locks
The pre-seeding of
entry->sizefrom disk at line 1358-1362 addresses the startup concern where pre-existing files might exceedmax_size.
1482-1487: fsync is correctly gated to rotation-enabled mode only.The
fsync()call is now conditional onfiles_rotation == FLB_TRUE && max_size > 0, avoiding the performance penalty when rotation is disabled. This addresses the concern about unconditional fsync on every flush.
1757-1771: Configuration entries are well-designed with backward-compatible defaults.The rotation feature is disabled by default (
files_rotation=false), preserving backward compatibility as required by the PR objectives. Default values are sensible:
max_size: 100MBmax_files: 7gzip: enabled by default for rotated filestests/runtime/CMakeLists.txt (1)
235-238: This comment is based on an incorrect analysis of the Windows code handling.The Windows code in
out_file_logrotate.cis not dead code—it's conditionally compiled using#ifdef FLB_SYSTEM_WINDOWS. All Windows-specific sections are properly guarded by preprocessor directives (lines 82, 212, 262, etc.), so they will be excluded during compilation on all platforms where this test actually runs. The test placement in theNOT FLB_SYSTEM_WINDOWSblock is intentional and correct, as it matches the platform support of theout_fileplugin itself.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Signed-off-by: SagiROosto <[email protected]>
SagiROosto
force-pushed
the
out_file-rotation
branch
from
9573977 to
d877ff2
Compare
2 participants
This PR present a new rotation feature to
out_file. This will answer #7572 question and solve #7541 suggestion.This feature is highly required in bare-metal or physical machines where you have finite and limited storage.
The suggested code disables the rotation feature by default to keep backward compatibility without affecting existing plugin users.
(this replaces closed #11110 PR)
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.