tail: New keep_file_handle and fstat_interval_nsec options for tail input #11151
Conversation
…nput Signed-off-by: David Garcia <[email protected]>
david-garcia-garcia
requested review from
cosmo0920 and
edsiper
as code owners
david-garcia-garcia
had a problem deploying
to
pr
GitHub Actions
Failure
david-garcia-garcia
had a problem deploying
to
pr
GitHub Actions
Failure
david-garcia-garcia
had a problem deploying
to
pr
GitHub Actions
Failure
WalkthroughAdds configurable file-handle management to the in_tail plugin: new options Changes
Sequence Diagram(s)sequenceDiagram
participant Config
participant TailFile
participant Backend
participant FS
rect rgba(45, 125, 255, 0.06)
Config->>TailFile: set keep_file_handle (true/false) & fstat_interval
end
alt keep_file_handle = true
Backend->>TailFile: request stat/process
TailFile->>FS: fstat(fd) -- via flb_tail_file_stat
FS-->>TailFile: stat
TailFile-->>Backend: stat result (rotation checks enabled)
Backend->>TailFile: lseek/update offset as required
else keep_file_handle = false
Backend->>TailFile: ensure_open_handle()
TailFile->>FS: open()/lseek() as needed
TailFile->>FS: fstat(opened fd) -- via flb_tail_file_stat
FS-->>TailFile: stat
TailFile-->>Backend: data (rotation checks skipped until reopen)
TailFile->>FS: close() -- close_handle_during_tail
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-10-23T07:43:16.216ZApplied to files:
🧬 Code graph analysis (1)tests/runtime/in_tail.c (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (30)
🔇 Additional comments (5)
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 |
…nput Signed-off-by: David Garcia <[email protected]>
david-garcia-garcia
temporarily deployed
to
pr
GitHub Actions
Inactive
david-garcia-garcia
temporarily deployed
to
pr
GitHub Actions
Inactive
david-garcia-garcia
temporarily deployed
to
pr
GitHub Actions
Inactive
![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
fluent-bit/plugins/in_tail/tail_fs_inotify.c
Lines 135 to 139 in 0bc15c8
Restore external visibility for flb_tail_file_name
This change makes flb_tail_file_name static inside tail_file.c and removes the declaration from the header, but callers in other compilation units (tail_fs_inotify.c and tail_fs_stat.c) still invoke it. Because the symbol now has internal linkage, those references no longer resolve and the build fails with an undefined symbol for flb_tail_file_name. Either keep the function exported or replace the cross-file uses.
ℹ️ 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/in_tail/tail_file.c (1)
2071-2170: Restore external linkage for flb_tail_file_name().Other units call this symbol; making it static breaks linking. Change to non‑static (paired with header re‑export).
- static char *flb_tail_file_name(struct flb_tail_file *file) + char *flb_tail_file_name(struct flb_tail_file *file)
🧹 Nitpick comments (6)
tests/runtime/in_tail.c (2)
1388-1462: Reduce test flakiness: wait for outputs instead of fixed sleeps.Replace the three 500 ms sleeps with wait_num_with_timeout(5000, &num) loops to make the test robust on slow CI.
- /* waiting to flush */ - flb_time_msleep(500); + /* wait up to 5s for at least one output */ + wait_num_with_timeout(5000, &num); ... - /* waiting to flush */ - flb_time_msleep(500); + wait_num_with_timeout(5000, &num); ... - /* waiting to flush */ - flb_time_msleep(500); + wait_num_with_timeout(5000, &num);
1464-1539: Apply the same non‑flaky wait strategy to the “keep_file_handle=true” test.Mirror the wait_num_with_timeout change here to avoid time‑sensitive failures.
plugins/in_tail/tail.c (1)
622-628: Consider parsing fstat_interval via existing time utility.You advertise “s/ms/us/ns”; instead of manual parsing in tail_config.c, you could use flb_utils_time_split() for consistent semantics with refresh_interval. Not a blocker.
plugins/in_tail/tail_config.c (1)
194-233: Leverage flb_utils_time_split for fstat_interval.You can simplify and unify parsing by using flb_utils_time_split(tmp, &sec, &nsec) like refresh_interval, then compute ctx->fstat_interval_nsec. This removes unit parsing code and edge cases.
- tmp = flb_input_get_property("fstat_interval", ins); - if (tmp) { - char *end = NULL; - double val = strtod(tmp, &end); - uint64_t mult = 1000000000ULL; /* default: seconds */ - ... - ctx->fstat_interval_nsec = (uint64_t) nsec_d; - ... - } + tmp = flb_input_get_property("fstat_interval", ins); + if (tmp) { + int sec; long nsec; + if (flb_utils_time_split(tmp, &sec, &nsec) == 0 && (sec > 0 || nsec > 0)) { + ctx->fstat_interval_nsec = (uint64_t)sec * 1000000000ULL + (uint64_t)nsec; + if (ctx->fstat_interval_nsec <= 1000000ULL) { + flb_plg_warn(ctx->ins, "very low fstat_interval (%" PRIu64 " ns) may cause high CPU usage", + ctx->fstat_interval_nsec); + } + } + else { + flb_plg_error(ctx->ins, "invalid 'fstat_interval' value (%s)", tmp); + flb_tail_config_destroy(ctx); + return NULL; + } + }plugins/in_tail/tail_fs_stat.c (1)
168-190: Avoid redundant real‑name lookup.You call flb_tail_file_name(file) but then rely on flb_tail_file_is_rotated(), which does the lookup again. Remove the extra allocation.
- name = flb_tail_file_name(file); - if (!name) { - flb_plg_debug(ctx->ins, "could not resolve %s, removing", file->name); - flb_tail_file_remove(file); - continue; - } ... - if (flb_tail_file_is_rotated(ctx, file) == FLB_TRUE) { + if (flb_tail_file_is_rotated(ctx, file) == FLB_TRUE) { flb_tail_file_rotated(file); } - flb_free(name);plugins/in_tail/tail_file.c (1)
1635-1666: Optional: set pending_bytes on truncation.After truncation branch, pending_bytes may carry stale value; set it explicitly from current st/offset.
if (size_delta < 0) { ... - /* Update offset in the database file */ + /* Sync pending_bytes after truncation */ + file->pending_bytes = 0; + /* Update offset in the database file */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
plugins/in_tail/tail.c(5 hunks)plugins/in_tail/tail_config.c(2 hunks)plugins/in_tail/tail_config.h(1 hunks)plugins/in_tail/tail_file.c(23 hunks)plugins/in_tail/tail_file.h(1 hunks)plugins/in_tail/tail_fs_inotify.c(3 hunks)plugins/in_tail/tail_fs_stat.c(5 hunks)tests/runtime/in_tail.c(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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:
plugins/in_tail/tail_fs_inotify.cplugins/in_tail/tail_fs_stat.cplugins/in_tail/tail_file.c
🧬 Code graph analysis (7)
plugins/in_tail/tail_file.h (1)
plugins/in_tail/tail_file.c (4)
flb_tail_file_ensure_open_handle(62-90)flb_tail_file_stat(93-101)flb_tail_file_close_handle(104-110)flb_tail_file_close_handle_during_tail(113-118)
plugins/in_tail/tail_fs_inotify.c (1)
plugins/in_tail/tail_file.c (1)
flb_tail_file_stat(93-101)
plugins/in_tail/tail.c (1)
plugins/in_tail/tail_file.c (1)
flb_tail_file_stat(93-101)
tests/runtime/in_tail.c (1)
src/flb_lib.c (2)
flb_input_set(305-335)flb_output_set(520-551)
plugins/in_tail/tail_config.c (1)
src/flb_input.c (1)
flb_input_get_property(776-780)
plugins/in_tail/tail_fs_stat.c (2)
plugins/in_tail/tail_file.c (5)
flb_tail_file_stat(93-101)flb_tail_file_name(2079-2170)flb_tail_file_remove(1519-1594)flb_tail_file_is_rotated(1948-2019)flb_tail_file_rotated(2197-2273)src/flb_input.c (1)
flb_input_set_collector_time(1685-1704)
plugins/in_tail/tail_file.c (2)
include/fluent-bit/flb_mem.h (1)
flb_free(126-128)src/flb_compression.c (1)
flb_decompression_context_create(151-219)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: pr-compile-centos-7
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
- GitHub Check: pr-compile-without-cxx (3.31.6)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
- GitHub Check: PR - fuzzing test
🔇 Additional comments (13)
plugins/in_tail/tail_config.h (1)
110-113: LGTM: new config fields look correct and well‑placed.No structural risks; types are appropriate for ns granularity and boolean.
tests/runtime/in_tail.c (1)
1562-1564: Good fix: use input fd for flb_input_set.Corrects misuse of ctx->o_ffd.
plugins/in_tail/tail.c (3)
81-89: Stat wrapper usage is correct.Switching to flb_tail_file_stat() centralizes status retrieval and respects closed FDs.
327-335: Gating link-rotation checks by keep_file_handle is appropriate.Rotation detection relies on an open FD; skipping it when handles aren’t kept avoids invalid paths.
730-736: Option name and default read well.keep_file_handle default true maintains current behavior; help text is clear.
plugins/in_tail/tail_fs_inotify.c (2)
218-233: Stat wrapper adoption looks good.Consistent with new helpers; error handling remains correct.
259-273: Truncation handling respects keep_file_handle.Seeking vs resetting offset matches the configured policy.
plugins/in_tail/tail_config.c (1)
109-111: Sane defaults.Default keep_file_handle=true and 250ms poll are reasonable.
plugins/in_tail/tail_fs_stat.c (3)
102-107: Stat wrapper adoption is correct.Aligns with closed‑FD behavior and unified error handling.
127-154: Truncation handling respects keep_file_handle.Seeking vs offset reset mirrors inotify path; DB offset update preserved.
205-213: Nice: timer now uses configured fstat_interval.Converts ns to (sec, nsec) for collector; good.
plugins/in_tail/tail_file.c (2)
61-90: Open‑handle helper looks good.Opens on demand and seeks to current offset; returns FLB_TAIL_ERROR consistently.
1891-1943: Closing on non‑kept handles is correctly placed.You close after processing, EOF, and errors; matches policy.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugins/in_tail/tail_file.c (1)
1277-1294: Fix memory leak: file->name is duplicated twice.This issue was previously identified. Line 1281 allocates
file->namewithflb_strdup(path), and then line 1307'sflb_tail_file_name_dup()allocates it again (see line 2174) without freeing the first allocation.Apply the fix suggested in the previous review:
file->name = flb_strdup(path); if (!file->name) { flb_errno(); goto error; } file->name_len = strlen(file->name); /* Open file handle first - flb_tail_file_name_dup() needs it open */ ret = flb_tail_file_ensure_open_handle(file); if (ret != 0) { flb_free(file->name); file->name = NULL; goto error; } + /* Avoid leaking the temporary name; flb_tail_file_name_dup() will set it again */ + flb_free(file->name); + file->name = NULL; + file->name_len = 0; + /* * Duplicate string into 'file' structure, the called function * take cares to resolve real-name of the file in case we are * running in a non-Linux system. */ ret = flb_tail_file_name_dup(path, file);
🧹 Nitpick comments (1)
plugins/in_tail/tail_file.c (1)
1911-1945: Consider consolidating redundant close calls.Lines 1911, 1921, 1936, and 1942 all call
flb_tail_file_close_handle_during_tail(). While the function is idempotent and safe to call multiple times, the calls at lines 1936 and 1942 are in the same error path and could be consolidated.Example refactor:
else { /* error */ flb_errno(); flb_plg_error(ctx->ins, "error reading %s", file->name); - - /* Close file handle if keep_file_handle is false */ - flb_tail_file_close_handle_during_tail(file); - - return FLB_TAIL_ERROR; - } - - /* Close file handle if keep_file_handle is false */ - flb_tail_file_close_handle_during_tail(file); + flb_tail_file_close_handle_during_tail(file); + return FLB_TAIL_ERROR; + } + return FLB_TAIL_ERROR;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugins/in_tail/tail_file.c(23 hunks)plugins/in_tail/tail_file.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/in_tail/tail_file.h
🧰 Additional context used
🧠 Learnings (1)
📚 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:
plugins/in_tail/tail_file.c
🧬 Code graph analysis (1)
plugins/in_tail/tail_file.c (2)
include/fluent-bit/flb_mem.h (1)
flb_free(126-128)src/flb_compression.c (1)
flb_decompression_context_create(151-219)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (28)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-without-cxx (3.31.6)
- GitHub Check: pr-compile-centos-7
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COMPILER_STRICT_POINTER_TYPES=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
- GitHub Check: PR - fuzzing test
🔇 Additional comments (7)
plugins/in_tail/tail_file.c (7)
61-119: LGTM: Clean abstraction for file handle management.The new helper functions provide a clean abstraction for managing file handles with proper error handling and conditional behavior based on the
keep_file_handleconfiguration.
1313-1322: LGTM: Decompression context initialization.The decompression context creation is correctly positioned after the file handle is opened and the name is duplicated.
1637-1649: LGTM: Correct truncation handling for both handle modes.The logic correctly handles file truncation: when
keep_file_handleis true, it seeks the open handle; when false, it updates the offset knowing thatflb_tail_file_ensure_open_handle()will seek correctly on the next open.
2043-2049: LGTM: Rotation detection correctly gated.Rotation detection is appropriately conditional on
keep_file_handlebeing enabled, as the detection mechanism requires an open file descriptor.
2207-2214: LGTM: Good defensive check.The defensive check ensures
flb_tail_file_rotated()is only called whenkeep_file_handleis enabled, preventing misuse.
2074-2078: LGTM: Clarified function requirements.The updated comment clearly documents that this function requires an open file descriptor and should only be called when
keep_file_handleis enabled.
2282-2336: LGTM: Consistent use of stat abstraction.The purge logic correctly uses the
flb_tail_file_stat()wrapper for consistent file status checking.
david-garcia-garcia
temporarily deployed
to
pr
GitHub Actions
Inactive
david-garcia-garcia
temporarily deployed
to
pr
GitHub Actions
Inactive
david-garcia-garcia
temporarily deployed
to
pr
GitHub Actions
Inactive
david-garcia-garcia
temporarily deployed
to
pr
GitHub Actions
Inactive
david-garcia-garcia
temporarily deployed
to
pr
GitHub Actions
Inactive
david-garcia-garcia
temporarily deployed
to
pr
GitHub Actions
Inactive
david-garcia-garcia
temporarily deployed
to
pr
GitHub Actions
Inactive
david-garcia-garcia
temporarily deployed
to
pr
GitHub Actions
Inactive
david-garcia-garcia
temporarily deployed
to
pr
GitHub Actions
Inactive
david-garcia-garcia
temporarily deployed
to
pr
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/in_tail/tail_fs_stat.c (1)
164-184: Critical: Rotation detection should be conditional onkeep_file_handle.The rotation detection logic calls
flb_tail_file_name(file)which discovers the file name from an open file descriptor. Whenkeep_file_handleisFLB_FALSE,file->fdis-1, causingflb_tail_file_nameto fail and returnNULL. This results in all files being incorrectly removed with "could not resolve" errors.The AI summary states: "Rotation checks now run only when keep_file_handle is true," but the code doesn't implement this conditional.
Wrap the rotation detection in a
keep_file_handlecheck:file->pending_bytes = (st.st_size - file->offset); tail_signal_pending(ctx); } else { file->pending_bytes = 0; } - /* Discover the current file name for the open file descriptor */ - name = flb_tail_file_name(file); - if (!name) { - flb_plg_debug(ctx->ins, "could not resolve %s, removing", file->name); - flb_tail_file_remove(file); - continue; - } - - /* - * Check if file still exists. This method requires explicity that the - * user is using an absolute path, otherwise we will be rotating the - * wrong file. - * - * flb_tail_target_file_name_cmp is a deeper compare than - * flb_tail_file_name_cmp. If applicable, it compares to the underlying - * real_name of the file. - */ - if (flb_tail_file_is_rotated(ctx, file) == FLB_TRUE) { - flb_tail_file_rotated(file); - } - flb_free(name); + /* Rotation detection only works when keeping file handles open */ + if (ctx->keep_file_handle == FLB_TRUE) { + /* Discover the current file name for the open file descriptor */ + name = flb_tail_file_name(file); + if (!name) { + flb_plg_debug(ctx->ins, "could not resolve %s, removing", file->name); + flb_tail_file_remove(file); + continue; + } + + /* + * Check if file still exists. This method requires explicity that the + * user is using an absolute path, otherwise we will be rotating the + * wrong file. + * + * flb_tail_target_file_name_cmp is a deeper compare than + * flb_tail_file_name_cmp. If applicable, it compares to the underlying + * real_name of the file. + */ + if (flb_tail_file_is_rotated(ctx, file) == FLB_TRUE) { + flb_tail_file_rotated(file); + } + flb_free(name); + } }
🧹 Nitpick comments (1)
plugins/in_tail/tail_fs_stat.c (1)
198-203: Consider using explicit time_t cast for clarity.The interval calculation logic is correct. The cast to
(int)for seconds works but is slightly unusual sinceflb_input_set_collector_timeexpectstime_t. While unlikely to cause issues in practice (would require extremely large intervals), usingtime_tdirectly would be clearer./* Set a manual timer to collect events using configured interval */ /* Convert nanoseconds to seconds and nanoseconds for the API */ ret = flb_input_set_collector_time(in, tail_fs_event, - (int)(ctx->fstat_interval_nsec / 1000000000L), + (time_t)(ctx->fstat_interval_nsec / 1000000000L), (long)(ctx->fstat_interval_nsec % 1000000000L), config);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/in_tail/tail_fs_stat.c(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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:
plugins/in_tail/tail_fs_stat.c
🧬 Code graph analysis (1)
plugins/in_tail/tail_fs_stat.c (3)
plugins/in_tail/tail_file.c (1)
flb_tail_file_stat(93-101)src/flb_input.c (1)
flb_input_set_collector_time(1685-1704)plugins/in_tail/tail_fs_inotify.c (1)
tail_fs_event(166-311)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (31)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COMPILER_STRICT_POINTER_TYPES=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
- GitHub Check: PR - fuzzing test
- GitHub Check: pr-compile-without-cxx (3.31.6)
- GitHub Check: pr-compile-centos-7
🔇 Additional comments (2)
plugins/in_tail/tail_fs_stat.c (2)
65-65: LGTM!Correct usage of the
flb_tail_file_statwrapper, which handles both open (fstat) and closed (stat) file descriptor scenarios based on thekeep_file_handleconfiguration.
102-104: LGTM!The stat wrapper usage is correct, and the error message improvement (adding "check:" prefix) helps distinguish this stat error context from others in the logs.
| /* If keeping handle open, it's already open but at wrong offset - seek to beginning */ | ||
| if (ctx->keep_file_handle == FLB_TRUE) { | ||
| offset = lseek(file->fd, 0, SEEK_SET); | ||
| if (offset == -1) { | ||
| flb_errno(); | ||
| return -1; | ||
| } | ||
| file->offset = offset; | ||
| } | ||
| else { | ||
| /* If not keeping handle open, just update offset - handle will be opened/seeks correctly later */ | ||
| file->offset = 0; | ||
| } |
There was a problem hiding this comment.
Consider continuing to next file instead of aborting entire check on lseek failure.
The truncation handling logic is correct for both modes. However, when lseek fails (line 134), returning -1 aborts the entire tail_fs_check function, preventing all remaining files in the list from being checked. A single file's lseek failure shouldn't stop monitoring other files.
Consider this alternative:
if (ctx->keep_file_handle == FLB_TRUE) {
offset = lseek(file->fd, 0, SEEK_SET);
if (offset == -1) {
flb_errno();
- return -1;
+ flb_plg_error(ctx->ins, "failed to seek after truncation: %s", file->name);
+ flb_tail_file_remove(file);
+ continue;
}
file->offset = offset;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /* If keeping handle open, it's already open but at wrong offset - seek to beginning */ | |
| if (ctx->keep_file_handle == FLB_TRUE) { | |
| offset = lseek(file->fd, 0, SEEK_SET); | |
| if (offset == -1) { | |
| flb_errno(); | |
| return -1; | |
| } | |
| file->offset = offset; | |
| } | |
| else { | |
| /* If not keeping handle open, just update offset - handle will be opened/seeks correctly later */ | |
| file->offset = 0; | |
| } | |
| /* If keeping handle open, it's already open but at wrong offset - seek to beginning */ | |
| if (ctx->keep_file_handle == FLB_TRUE) { | |
| offset = lseek(file->fd, 0, SEEK_SET); | |
| if (offset == -1) { | |
| flb_errno(); | |
| flb_plg_error(ctx->ins, "failed to seek after truncation: %s", file->name); | |
| flb_tail_file_remove(file); | |
| continue; | |
| } | |
| file->offset = offset; | |
| } | |
| else { | |
| /* If not keeping handle open, just update offset - handle will be opened/seeks correctly later */ | |
| file->offset = 0; | |
| } |
🤖 Prompt for AI Agents
In plugins/in_tail/tail_fs_stat.c around lines 129-141, when lseek(file->fd, 0,
SEEK_SET) fails the code currently calls flb_errno() and returns -1 which aborts
checking all remaining files; instead, log the error (flb_errno()) and skip this
file by continuing the loop so other files are still processed. Replace the
early return with logic to continue to the next file (do not modify file->offset
on failure), ensuring any necessary cleanup for this iteration is preserved.
1 participant
Adds new configuration options keep_file_handle and fstat_interval_nsec for tail input.
The purpose of this is to prevent file handles from being kept open while files are tailed. The motivation is to support tailing files that have an SMB/SAMBA underlying storage. SMB/SAMBA implementation will prevent tailed files from being deleted if a file handle is open.
NOTICE: when keep_file_handle is set to false, log rotation detection does not work as it requires an active handle to figure out where the original file was rotated to.
Fixes #11147
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
Refactor
Tests