in_tail: make stale database cleanup safe for shared databases - #12242
in_tail: make stale database cleanup safe for shared databases#12242Bing-Wang-derbysoft wants to merge 3 commits into
Conversation
Signed-off-by: Bing Wang <bing.wang@derbysoft.net>
Signed-off-by: Bing Wang <bing.wang@derbysoft.net>
📝 WalkthroughWalkthroughThe tail plugin moves SQLite stale-file cleanup to the pre-run callback. Cleanup checks file paths and inodes, preserves records monitored by matching inputs, and deletes records for missing files. Runtime tests cover replacement, rename, and shared-database scenarios. ChangesTail database cleanup
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Tail as Tail pre-run callback
participant Cleanup as flb_tail_db_cleanup
participant SQLite as SQLite database
participant Filesystem as Filesystem
Tail->>Cleanup: Check stale records
Cleanup->>SQLite: Select file records
SQLite-->>Cleanup: Return path and inode data
Cleanup->>Filesystem: Stat stored paths
Filesystem-->>Cleanup: Return file status and inode
Cleanup->>SQLite: Delete records for missing files
Cleanup-->>Tail: Return cleanup status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 3b22f9e0c2
ℹ️ 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
🧹 Nitpick comments (2)
tests/runtime/in_tail.c (2)
2932-2957: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReport the reason the check failed.
Every failure mode returns
FLB_FALSE: open failure, prepare failure, no rows, name mismatch, inode mismatch, and extra rows. The caller prints one generic message. A CI failure then gives no signal about which condition occurred.Add
TEST_MSGoutput at each failure point so the test log identifies the actual database state.♻️ Proposed diagnostics
ret = sqlite3_step(stmt); if (ret != SQLITE_ROW) { + TEST_MSG("no row found in in_tail_files, ret=%d", ret); goto cleanup; } name = sqlite3_column_text(stmt, 0); if (name == NULL || strcmp((const char *) name, file_path) != 0 || sqlite3_column_int64(stmt, 1) != (sqlite3_int64) inode) { + TEST_MSG("row mismatch: name=%s expect=%s inode=%" PRId64 + " expect=%" PRIu64, + name ? (const char *) name : "(null)", file_path, + (int64_t) sqlite3_column_int64(stmt, 1), inode); goto cleanup; } ret = sqlite3_step(stmt); if (ret == SQLITE_DONE) { found = FLB_TRUE; } + else { + TEST_MSG("unexpected extra rows in in_tail_files"); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/runtime/in_tail.c` around lines 2932 - 2957, The SQLite verification logic should report the specific failure before each cleanup path instead of returning only FLB_FALSE. Add TEST_MSG diagnostics in the sqlite3_open, sqlite3_prepare_v2, initial sqlite3_step, name/inode validation, and final sqlite3_step branches, including relevant return values or database-state details where available; preserve the existing cleanup and found-result behavior.
3181-3190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the database still holds both file records after the restart.
This test currently infers offset preservation from the output count. That is an indirect signal. The regression in issue
#8928is a database-layer defect: cleanup deleted the row owned by the other tail input.Add a direct check that
in_tail_filesstill contains two rows, one per file, after the secondflb_start. The check pins the failure to the database layer and does not depend on flush timing.
tail_db_contains_filecannot be reused here because it requires exactly one row. Add a small row-count helper next to it.♻️ Proposed helper and assertion
/* place next to tail_db_contains_file */ static int tail_db_count_files(const char *db_path) { int count = -1; int ret; sqlite3 *db = NULL; sqlite3_stmt *stmt = NULL; ret = sqlite3_open(db_path, &db); if (ret != SQLITE_OK) { goto cleanup; } ret = sqlite3_prepare_v2(db, "SELECT COUNT(*) FROM in_tail_files;", -1, &stmt, NULL); if (ret != SQLITE_OK) { goto cleanup; } if (sqlite3_step(stmt) == SQLITE_ROW) { count = sqlite3_column_int(stmt, 0); } cleanup: if (stmt != NULL) { sqlite3_finalize(stmt); } if (db != NULL) { sqlite3_close(db); } return count; }flb_time_msleep(500); num = get_output_num(); if (!TEST_CHECK(num == 2)) { TEST_MSG("num error. expect=2 got=%d", num); } + ret = tail_db_count_files(db); + if (!TEST_CHECK(ret == 2)) { + TEST_MSG("db record count error. expect=2 got=%d", ret); + } + test_tail_ctx_destroy(ctx); unlink(db); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/runtime/in_tail.c` around lines 3181 - 3190, Add a tail_db_count_files helper next to tail_db_contains_file that queries COUNT(*) from in_tail_files and safely cleans up SQLite resources. After the second flb_start and before relying on output timing, assert that the helper returns exactly 2 rows, preserving the existing output-count check as applicable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/in_tail/tail_config.c`:
- Around line 476-480: Update the flb_tail_config_create cleanup path around
flb_tail_db_cleanup so a nonzero cleanup result is logged but does not destroy
ctx or return NULL. Continue tail input initialization after stale database
cleanup failures, preserving normal handling for successful cleanup.
---
Nitpick comments:
In `@tests/runtime/in_tail.c`:
- Around line 2932-2957: The SQLite verification logic should report the
specific failure before each cleanup path instead of returning only FLB_FALSE.
Add TEST_MSG diagnostics in the sqlite3_open, sqlite3_prepare_v2, initial
sqlite3_step, name/inode validation, and final sqlite3_step branches, including
relevant return values or database-state details where available; preserve the
existing cleanup and found-result behavior.
- Around line 3181-3190: Add a tail_db_count_files helper next to
tail_db_contains_file that queries COUNT(*) from in_tail_files and safely cleans
up SQLite resources. After the second flb_start and before relying on output
timing, assert that the helper returns exactly 2 rows, preserving the existing
output-count check as applicable.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e4c9478-76e3-4219-976a-4c8a47798c58
📒 Files selected for processing (6)
plugins/in_tail/tail.cplugins/in_tail/tail_config.cplugins/in_tail/tail_db.cplugins/in_tail/tail_db.hplugins/in_tail/tail_sql.htests/runtime/in_tail.c
💤 Files with no reviewable changes (1)
- plugins/in_tail/tail.c
Signed-off-by: Bing Wang <bing.wang@derbysoft.net>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/runtime/in_tail.c (1)
3146-3162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the database state and remove the renamed file at the end.
The test verifies the emitted record count but not the database content. The PR adds
tail_db_contains_file. Use it after the restart to confirm that the record for the renamed path survived cleanup with the original inode. That check fails if cleanup deletes the record, which is the exact regression this PR fixes; the record-count check alone can pass for other reasons.The test also leaves
test_db_renamed.log.1on disk. The file is removed at the start of the test, so reruns work, but removing it at the end keeps the working directory clean for other tests that use glob paths.💚 Proposed test additions
num = get_output_num(); if (!TEST_CHECK(num == 1)) { TEST_MSG("num error after append. expect=1 got=%d", num); } + ret = tail_db_contains_file(db, renamed_file[0], inode); + if (!TEST_CHECK(ret == FLB_TRUE)) { + TEST_MSG("database record for the renamed file was not preserved"); + } + test_tail_ctx_destroy(ctx); + unlink(renamed_file[0]); unlink(db); }Capture the inode with
stat()before the rename to pass it totail_db_contains_file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/runtime/in_tail.c` around lines 3146 - 3162, Update the rename/restart test around test_tail_ctx_destroy to capture the original file inode with stat() before renaming, then call tail_db_contains_file after the restart to assert the renamed path remains associated with that inode. Retain the existing output-count assertions and unlink test_db_renamed.log.1 during final cleanup.plugins/in_tail/tail_db.c (1)
273-281: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider wrapping the delete loop in a single transaction.
Each
flb_tail_db_file_delete_by_idcall runs in its own implicit transaction. Withdb.sync fulland a database that holds many stale rows, startup performs one durable commit per deleted row. A single explicit transaction around the loop reduces startup latency and makes the cleanup atomic.This is optional if the expected stale-row count is small.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/in_tail/tail_db.c` around lines 273 - 281, Wrap the stale-file deletion loop in the surrounding cleanup flow with one explicit database transaction, using the transaction helpers already available in the tail DB implementation. Commit once after all flb_tail_db_file_delete_by_id calls succeed, roll back on any failure before cleanup, and preserve deleted_count and existing error propagation.plugins/in_tail/tail.c (1)
490-521: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffOptional: the monitored-inode lookup is linear per database row.
flb_tail_db_cleanupcalls this callback for each stale-candidate row, and each call walks every matching input and every tracked file. The total cost isrows x inputs x files. For a deployment with thousands of tracked files and a large database this adds measurable startup time. If that scale is expected, build a hash set of monitored inodes once inin_tail_pre_runand pass it asdata.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/in_tail/tail.c` around lines 490 - 521, Optimize tail database cleanup by building a hash set of monitored inode values once during in_tail_pre_run, then pass that set to flb_tail_db_cleanup and tail_db_inode_is_monitored instead of repeatedly traversing matching inputs and files for each database row. Update the callback to perform constant-time inode membership checks and ensure the set is released after cleanup.plugins/in_tail/tail_db.h (1)
45-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the stale-file cleanup callback contract.
flb_tail_db_cleanupnow takes a callback that must returnFLB_TRUEwheninodeis still monitored. Add a short contract comment above the callback typedef inplugins/in_tail/tail_db.h.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/in_tail/tail_db.h` around lines 45 - 47, Add a concise contract comment above the inode monitoring callback typedef in tail_db.h, documenting that the callback must return FLB_TRUE when the provided inode is still monitored. Leave the flb_tail_db_cleanup declaration unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/in_tail/tail.c`:
- Around line 512-517: The monitored-inode check in tail_db_inode_is_monitored
must include both tail_ctx->files_static and tail_ctx->files_event so promoted
files remain protected; update plugins/in_tail/tail.c lines 512-517 accordingly.
Also verify in plugins/in_tail/tail.c lines 524-543 that all tail inputs sharing
the database handler complete discovery before cb_pre_run, and move cleanup
later if necessary so every file list is populated before stale records are
removed.
---
Nitpick comments:
In `@plugins/in_tail/tail_db.c`:
- Around line 273-281: Wrap the stale-file deletion loop in the surrounding
cleanup flow with one explicit database transaction, using the transaction
helpers already available in the tail DB implementation. Commit once after all
flb_tail_db_file_delete_by_id calls succeed, roll back on any failure before
cleanup, and preserve deleted_count and existing error propagation.
In `@plugins/in_tail/tail_db.h`:
- Around line 45-47: Add a concise contract comment above the inode monitoring
callback typedef in tail_db.h, documenting that the callback must return
FLB_TRUE when the provided inode is still monitored. Leave the
flb_tail_db_cleanup declaration unchanged.
In `@plugins/in_tail/tail.c`:
- Around line 490-521: Optimize tail database cleanup by building a hash set of
monitored inode values once during in_tail_pre_run, then pass that set to
flb_tail_db_cleanup and tail_db_inode_is_monitored instead of repeatedly
traversing matching inputs and files for each database row. Update the callback
to perform constant-time inode membership checks and ensure the set is released
after cleanup.
In `@tests/runtime/in_tail.c`:
- Around line 3146-3162: Update the rename/restart test around
test_tail_ctx_destroy to capture the original file inode with stat() before
renaming, then call tail_db_contains_file after the restart to assert the
renamed path remains associated with that inode. Retain the existing
output-count assertions and unlink test_db_renamed.log.1 during final cleanup.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 53978600-14da-4f7a-acb4-1e1c6c03d753
📒 Files selected for processing (4)
plugins/in_tail/tail.cplugins/in_tail/tail_db.cplugins/in_tail/tail_db.htests/runtime/in_tail.c
1 participant
Summary
Make Tail database stale-file cleanup operate on the database as a whole instead
of on the files discovered by each individual Tail input.
When multiple Tail inputs share one database, the previous startup cleanup could
delete valid offset records belonging to another input. After restarting Fluent
Bit, affected files could then be read again from the beginning.
The new cleanup:
Regression tests cover shared databases and replacing a file with a new inode at
the same path.
Fixes #8928.
Follow-up to #8062, which introduced startup cleanup of unmonitored database
entries. The affected behavior was released in Fluent Bit 3.0.2.
Testing
Example configuration
The shared database behavior was tested with two Tail inputs using the same
database:
Runtime tests
The runtime test binary and Fluent Bit executable were built in an Ubuntu 24.04
Docker environment.
Result:
Integration tests
Result:
The same integration scenarios were run under strict Valgrind validation:
Result:
VALGRIND_STRICT=1checks the generated Valgrind logs for definite, indirect,and possible leaks, as well as reported Valgrind errors.
Representative cleanup log output:
Amazon Linux 2023 manual verification
Additional functional testing was performed with:
The following scenarios passed:
Packaging
This change does not modify packaging, containers, or native binary contents.
ok-package-testlabelDocumentation
This change does not add or modify configuration options or public interfaces.
Backporting
This fixes behavior introduced by #8062 and released in Fluent Bit 3.0.2. It may
be suitable for backporting after the master PR is accepted.
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
Bug Fixes
Tests