out_file: add logs rotation feature#11352
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds size-based file rotation with optional streaming gzip compression to the file output plugin, plus per-file size tracking and locking, rotation/retention lifecycle, config schema extensions, and a comprehensive cross-platform 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🧪 Generate unit tests (beta)
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.
SagiROosto
force-pushed
the
out_file-rotation
branch
from
5502c25 to
633e5d9
Compare
|
Failure seems like fleaky CI not-related to actual changes |
SagiROosto
force-pushed
the
out_file-rotation
branch
from
633e5d9 to
f4a0773
Compare
There was a problem hiding this comment.
I think this is a strong feature addition overall, but I would be hesitant to merge it as-is. There are a couple of correctness issues that look important enough to fix before merge.
fwrite() error handling in the msgpack path looks broken
In the msgpack branch, the code does:
ret = fwrite(...);
if (ret < 0) {
...
}fwrite() does not return a negative value here, so this condition will never trigger. On write failure, this can turn into a loop that does not make forward progress.
I think this should be handled using something like ret == 0 && ferror(fp) instead.
This one feels blocker-level to me.
Plus, there's a curious question.
How do we handle multiple rotation within the same second?
And we might need to add a test for multiple rotations within the same second.
Signed-off-by: SagiROosto <sagi.rosenthal@oosto.com>
Signed-off-by: SagiROosto <sagi.rosenthal@oosto.com>
SagiROosto
force-pushed
the
out_file-rotation
branch
from
f4a0773 to
eb23273
Compare
well TBH if one reaches a case where the plugin rotates the log file twice in the same wall-clock second then there is probably have greater issues... |
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