chunks: extend metadata to support routing information (direct routes) #11097
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds persistent direct-route metadata to chunk headers and reader APIs: a feature-flags byte enables a null-terminated tag followed by a routing payload (route count, output IDs as 16/32-bit, optional labels). Writers persist routes; task creation restores and validates them and falls back to tag-based routing when needed. Changes
Sequence Diagram(s)sequenceDiagram
participant Writer as Input Log Writer
participant ChunkFS as Chunk (fs-backed)
participant Task as Task Creator
participant OutputReg as Output Registry
Writer->>Writer: collect direct route IDs & labels
Writer->>ChunkFS: flb_input_chunk_write_header_v2(tag, routes)
ChunkFS->>ChunkFS: store flags, null-terminated tag, routing payload
Note right of Task: on restart / task creation
Task->>ChunkFS: flb_input_chunk_has_direct_routes?
alt direct routes present
ChunkFS-->>Task: yes
Task->>ChunkFS: flb_input_chunk_get_direct_routes -> routes[]
loop per route
Task->>OutputReg: resolve by label or ID
OutputReg-->>Task: output instance or missing
end
Task->>Task: create INACTIVE routes for resolved outputs
else no/invalid direct routes
ChunkFS-->>Task: no/invalid
Task->>Task: fall back to tag-based routing
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 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.
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| if (direct_index < direct_count) { | ||
| direct_ids[direct_index] = (uint8_t) route_path->ins->id; | ||
| direct_index++; |
There was a problem hiding this comment.
When persisting direct routes the code stores each destination’s id in a uint8_t buffer (direct_ids[direct_index] = (uint8_t) route_path->ins->id). Output instance IDs are plain ints and are assigned sequentially without an 8‑bit limit, so deployments with 256+ outputs will wrap IDs when writing metadata. After a restart these truncated values can either fail to resolve or resolve to the wrong output, causing chunks to be routed incorrectly or silently fall back to default routing. Use a wider type (e.g., uint16_t or int) or validate that IDs fit in 8 bits before serializing.
Useful? React with 👍 / 👎.
![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
🧹 Nitpick comments (1)
CHUNKS.md (1)
100-117: Add a language hint to the new fenced block.markdownlint (MD040) flags this new fence; adding a language like
textkeeps the pipeline green without changing the rendering.-``` +```text ... -``` +```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHUNKS.md(1 hunks)include/fluent-bit/flb_input_chunk.h(3 hunks)lib/chunkio/src/cio_file.c(1 hunks)src/flb_input_chunk.c(8 hunks)src/flb_input_log.c(2 hunks)src/flb_task.c(3 hunks)tests/internal/CMakeLists.txt(1 hunks)tests/internal/input_chunk_routes.c(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/flb_input_log.c (3)
src/flb_routes_mask.c (1)
flb_routes_mask_get_bit(125-140)src/flb_input_chunk.c (2)
flb_input_chunk_write_header_v2(1074-1151)flb_input_chunk_get_name(2469-2475)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
src/flb_task.c (4)
src/flb_input_chunk.c (3)
flb_input_chunk_has_direct_routes(1153-1176)flb_input_chunk_get_direct_routes(1178-1235)flb_input_chunk_get_name(2469-2475)include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)src/flb_router_condition.c (1)
flb_router_chunk_context_destroy(187-190)src/flb_input.c (1)
flb_input_name(790-797)
include/fluent-bit/flb_input_chunk.h (1)
src/flb_input_chunk.c (3)
flb_input_chunk_write_header_v2(1074-1151)flb_input_chunk_has_direct_routes(1153-1176)flb_input_chunk_get_direct_routes(1178-1235)
src/flb_input_chunk.c (2)
src/flb_routes_mask.c (2)
flb_routes_mask_set_bit(76-91)flb_routes_mask_set_by_tag(31-67)include/fluent-bit/flb_mem.h (2)
flb_free(126-128)flb_calloc(84-96)
tests/internal/input_chunk_routes.c (2)
include/fluent-bit/flb_mem.h (1)
flb_free(126-128)src/flb_input_chunk.c (4)
flb_input_chunk_write_header_v2(1074-1151)flb_input_chunk_has_direct_routes(1153-1176)flb_input_chunk_get_direct_routes(1178-1235)flb_input_chunk_get_tag(2519-2536)
🪛 markdownlint-cli2 (0.18.1)
CHUNKS.md
100-100: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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)
- 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 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: run-ubuntu-unit-tests (-DFLB_SMALL=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 (-DSANITIZE_ADDRESS=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_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_JEMALLOC=On, 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_MEMORY=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_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 (-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_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-without-cxx (3.31.6)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 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 (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
- 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-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
- 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-centos-7
🔇 Additional comments (1)
lib/chunkio/src/cio_file.c (1)
1109-1109: Good catch on preserving the full content region.Switching the relocation to copy
cf->data_sizebytes fixes the data truncation that happened when shrinking metadata layouts. Thanks for tightening this up.
| if (flb_input_chunk_has_direct_routes(task_ic) == FLB_TRUE) { | ||
| stored_routes_result = flb_input_chunk_get_direct_routes(task_ic, | ||
| &direct_output_ids, | ||
| &direct_output_count); | ||
| if (stored_routes_result == 0 && direct_output_count > 0) { | ||
| stored_routes_valid = FLB_TRUE; | ||
| missing_output_id = 0; | ||
| for (direct_output_index = 0; | ||
| direct_output_index < direct_output_count; | ||
| direct_output_index++) { | ||
| stored_output = task_find_output_by_id(config, | ||
| direct_output_ids[direct_output_index]); | ||
| if (!stored_output) { | ||
| stored_routes_valid = FLB_FALSE; | ||
| missing_output_id = direct_output_ids[direct_output_index]; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (stored_routes_valid == FLB_TRUE) { | ||
| direct_count = 0; | ||
| stored_routes_alloc_failed = FLB_FALSE; | ||
| for (direct_output_index = 0; | ||
| direct_output_index < direct_output_count; | ||
| direct_output_index++) { | ||
| stored_output = task_find_output_by_id(config, | ||
| direct_output_ids[direct_output_index]); | ||
| if (!stored_output) { | ||
| continue; | ||
| } | ||
|
|
||
| route = flb_calloc(1, sizeof(struct flb_task_route)); | ||
| if (!route) { | ||
| flb_errno(); | ||
| stored_routes_alloc_failed = FLB_TRUE; | ||
| break; | ||
| } | ||
|
|
||
| route->status = FLB_TASK_ROUTE_INACTIVE; | ||
| route->out = stored_output; | ||
| mk_list_add(&route->_head, &task->routes); | ||
| direct_count++; | ||
| } | ||
|
|
||
| if (stored_routes_alloc_failed == FLB_TRUE) { | ||
| if (router_context_initialized) { | ||
| flb_router_chunk_context_destroy(&router_context); | ||
| router_context_initialized = FLB_FALSE; | ||
| } | ||
| if (direct_output_ids) { | ||
| flb_free(direct_output_ids); | ||
| } | ||
| task->event_chunk->data = NULL; | ||
| flb_task_destroy(task, FLB_TRUE); | ||
| return NULL; | ||
| } | ||
|
|
||
| if (direct_count > 0) { | ||
| stored_routes_used = FLB_TRUE; | ||
| } | ||
| } | ||
| else { | ||
| flb_warn("[task] input=%s/%s stored direct route id=%u not found for chunk %s, falling back to configured routes", | ||
| i_ins->p->name, | ||
| flb_input_name(i_ins), | ||
| missing_output_id, | ||
| flb_input_chunk_get_name(task_ic)); | ||
| } | ||
| } | ||
| else if (stored_routes_result == -2) { | ||
| flb_warn("[task] input=%s/%s invalid stored direct routing metadata for chunk %s, falling back to configured routes", | ||
| i_ins->p->name, | ||
| flb_input_name(i_ins), | ||
| flb_input_chunk_get_name(task_ic)); | ||
| } | ||
| } | ||
|
|
||
| if (stored_routes_used == FLB_TRUE) { |
There was a problem hiding this comment.
Stored direct route IDs need a stable key
Restoring the routes solely by matching the persisted uint8_t to o_ins->id assumes that output ids never change. In practice those ids are assigned sequentially when outputs are instantiated, so adding/removing/reordering outputs (a common config change between restarts) will shift the numbering. After such a change a backlog chunk will happily bind to whatever new output now owns the same numeric id, sending data to the wrong destination with no warning. We need to persist and validate a stable identifier (e.g., alias/name + plugin type hash) alongside the id, and refuse/directly fall back to normal routing when it no longer matches. Please extend the metadata format and the restore path accordingly (and cover the equivalent restoration in flb_input_chunk_map() too).
🤖 Prompt for AI Agents
In src/flb_task.c around lines 459-536, the current restore logic matches stored
direct routes only by persisted uint8_t output id which can change; update the
metadata to persist a stable key (e.g., output alias/name plus plugin-type hash)
alongside the numeric id, and change the restore loop to read both id and stable
key, locate the output by verifying the stable key matches the candidate output
(not just id), and if the stable key does not match treat it as a missing route
(log a warning and fall back to configured routes). Ensure backward
compatibility by continuing to accept legacy entries that only include numeric
id (but prefer failing safe if validation cannot be performed), update the code
paths that write route metadata to include the new stable key, mirror the same
validation logic in flb_input_chunk_map(), free any allocated memory on errors,
destroy router_context as before, and add a concise warning message when a
stored route is rejected due to stable-id mismatch so operators can detect
config drift.
edsiper
force-pushed
the
storage-metadata-for-routing
branch
from
a8b7f5d to
c9b7465
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/flb_task.c
Outdated
| static struct flb_output_instance *task_find_output_by_id(struct flb_config *config, | ||
| uint8_t id) | ||
| { | ||
| struct mk_list *head; | ||
| struct flb_output_instance *o_ins; | ||
|
|
||
| if (!config) { | ||
| return NULL; | ||
| } | ||
|
|
||
| mk_list_foreach(head, &config->outputs) { | ||
| o_ins = mk_list_entry(head, struct flb_output_instance, _head); | ||
| if (o_ins->id == id) { | ||
| return o_ins; | ||
| } |
There was a problem hiding this comment.
Direct-route metadata stores output IDs as 16-bit values (uint16_t), but the helper used during task recovery accepts a uint8_t. Any output whose ID exceeds 255 will be truncated when task_find_output_by_id searches for the persisted route, so chunks with valid stored routes will fail to bind to their original outputs and will fall back to tag routing (or be dropped) after restart. The lookup function should take at least a 16-bit or native int to match the stored metadata.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/flb_input_chunk.c (1)
919-994: ** Critical: Direct route restoration lacks stable output validation.**Lines 927-950 validate stored routes only by numeric output ID. Since IDs are reassigned sequentially on restart, config changes (reordering/replacing outputs) will cause backlog chunks to route to different destinations. The code must persist and validate a stable identifier (output alias/name + plugin type) alongside the numeric ID, rejecting routes when the stable identifier no longer matches.
Extend the routing metadata to include output stable identifiers, update the parse/validation logic to check them, and fall back to tag routing with a warning when validation fails. This mirrors the same issue in
src/flb_task.c(Lines 459-536).src/flb_task.c (2)
74-92: ** Critical: Output ID type mismatch causes truncation.**The helper accepts
uint8_t id(Line 75) but direct-route metadata stores 16-bit output IDs. Any output with ID ≥ 256 will be truncated during lookup, causing chunks to fail restoration or bind to the wrong output. Change the parameter to at leastuint16_torint.Apply this diff:
-static struct flb_output_instance *task_find_output_by_id(struct flb_config *config, - uint8_t id) +static struct flb_output_instance *task_find_output_by_id(struct flb_config *config, + uint16_t id) { struct mk_list *head; struct flb_output_instance *o_ins;
459-536: ** Critical: Stored routes lack stable output identifiers.**The restoration logic (Lines 469-476) validates outputs solely by numeric ID, which are reassigned sequentially each restart. Config changes that reorder/swap outputs will cause stored chunks to route to different destinations silently. The metadata must include a stable key (e.g., output alias/name + plugin type) and this code must verify it matches before accepting the stored route, falling back to tag routing on mismatch.
Extend the metadata format to persist output alias/name alongside IDs, update
flb_input_chunk_get_direct_routesto return stable identifiers, and add validation here that compares them. Log a warning when stored routes are rejected due to identifier mismatch.
🧹 Nitpick comments (1)
CHUNKS.md (1)
100-117: Specify language for fenced code block.The fenced code block should specify a language identifier for proper syntax highlighting.
Based on static analysis.
Apply this diff:
-``` +```text Metadata -- +---------+-------+
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHUNKS.md(1 hunks)include/fluent-bit/flb_input_chunk.h(3 hunks)lib/chunkio/src/cio_file.c(1 hunks)src/flb_input_chunk.c(8 hunks)src/flb_input_log.c(2 hunks)src/flb_task.c(3 hunks)tests/internal/CMakeLists.txt(1 hunks)tests/internal/input_chunk_routes.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- include/fluent-bit/flb_input_chunk.h
- tests/internal/CMakeLists.txt
🧰 Additional context used
🧬 Code graph analysis (4)
src/flb_input_log.c (3)
src/flb_routes_mask.c (1)
flb_routes_mask_get_bit(125-140)src/flb_input_chunk.c (2)
flb_input_chunk_write_header_v2(1075-1162)flb_input_chunk_get_name(2485-2491)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
src/flb_task.c (4)
src/flb_input_chunk.c (3)
flb_input_chunk_has_direct_routes(1164-1187)flb_input_chunk_get_direct_routes(1189-1251)flb_input_chunk_get_name(2485-2491)include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)src/flb_router_condition.c (1)
flb_router_chunk_context_destroy(187-190)src/flb_input.c (1)
flb_input_name(790-797)
tests/internal/input_chunk_routes.c (2)
include/fluent-bit/flb_mem.h (1)
flb_free(126-128)src/flb_input_chunk.c (4)
flb_input_chunk_write_header_v2(1075-1162)flb_input_chunk_has_direct_routes(1164-1187)flb_input_chunk_get_direct_routes(1189-1251)flb_input_chunk_get_tag(2535-2552)
src/flb_input_chunk.c (2)
src/flb_routes_mask.c (2)
flb_routes_mask_set_bit(76-91)flb_routes_mask_set_by_tag(31-67)include/fluent-bit/flb_mem.h (2)
flb_free(126-128)flb_calloc(84-96)
🪛 GitHub Actions: Run unit tests
lib/chunkio/src/cio_file.c
[error] 1-1: Unit test suite reported failures: 1 of 169 tests failed (flb-it-input_chunk_routes).
tests/internal/input_chunk_routes.c
[error] 109-109: Test 'flb-it-input_chunk_routes' failed. Test chunk_metadata_direct_routes... Check ret == CIO_OK... failed.
🪛 markdownlint-cli2 (0.18.1)
CHUNKS.md
100-100: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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). (3)
- GitHub Check: run-windows-unit-tests / call-build-windows-unit-test (Windows 64bit, x64, x64-windows-static, 3.31.6, windows-latest)
- GitHub Check: run-windows-unit-tests / call-build-windows-unit-test (Windows 32bit, x86, x86-windows-static, 3.31.6, windows-latest)
- GitHub Check: run-windows-unit-tests / call-build-windows-unit-test (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DC...
🔇 Additional comments (5)
lib/chunkio/src/cio_file.c (1)
1109-1109: LGTM: Correct data size used for content relocation.The change to use
cf->data_sizeinstead ofsizewhen moving content is correct. When metadata space is sufficient (meta_av >= size), we need to relocate the existing content data (whose size iscf->data_size), not the new metadata size.src/flb_input_log.c (1)
205-270: Direct routing persistence logic is well-structured.The implementation correctly:
- Pre-counts eligible routes before allocation
- Allocates appropriately-sized buffer for output IDs
- Casts output IDs to
uint16_tmatching the stored format (Line 248)- Calls
flb_input_chunk_write_header_v2to persist metadata- Frees allocated memory on all paths
- Logs warnings on write failure without blocking the operation
src/flb_input_chunk.c (2)
65-171: Metadata view implementation is well-structured.The new
flb_input_chunk_meta_viewstructure andinput_chunk_metadata_viewfunction provide a clean abstraction for parsing chunk metadata. The code correctly:
- Validates magic bytes
- Extracts tag with proper null-terminator handling
- Parses routing payload when flags indicate direct routes
- Handles legacy format gracefully (flags = 0)
1075-1251: Direct routing API functions are correctly implemented.The new API functions:
flb_input_chunk_write_header_v2: Properly encodes routing metadata with big-endian 16-bit IDsflb_input_chunk_has_direct_routes: Validates flags and payload presenceflb_input_chunk_get_direct_routes: Decodes routing payload with proper bounds checkingMemory management, error handling, and boundary validation are all correct.
tests/internal/input_chunk_routes.c (1)
108-109: I've completed a thorough analysis of the code path for this test. Here's what I found:Code Review Summary:
The test callscio_chunk_up_force()on a freshly created chunk with proper initialization. The function chain is:
cio_chunk_up_force()→cio_file_up_force()→_cio_file_up(ch, CIO_FALSE)_cio_file_up()opens the file withO_RDWR | O_CREAT, gets its size, and memory-maps it- For an empty file (size 0) with
CIO_OPEN_RWflag (which is set), it callscio_file_resize()to allocate space, thencio_file_native_map()Findings:
The code logic appears sound for the normal case. The test setup correctly:
- Deletes and recreates the test path
- Creates a fresh context, stream, and chunk
- Opens the chunk with proper flags
Potential Failure Points:
Ifcio_chunk_up_force()returns non-OK, it would be from:
cio_file_native_open()failure (file permissions or /tmp unavailable)cio_file_resize()failure (disk space or permissions)cio_file_native_map()failure (mmap syscall failure)Limitation:
I cannot compile or execute the test in the sandbox environment to reproduce the failure. The issue appears to be CI environment-specific or caused by a recent code change affecting file operations.
edsiper
force-pushed
the
storage-metadata-for-routing
branch
from
c9b7465 to
d42742b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHUNKS.md (1)
103-124: Add language identifier to fenced code block.The ASCII diagram fenced code block should specify a language identifier (even if it's just
text) to satisfy markdown linting rules and improve rendering consistency.Apply this diff:
-``` +```text Metadata -- +---------+-------+As per coding guidelines (static analysis hint MD040).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHUNKS.md(1 hunks)include/fluent-bit/flb_input_chunk.h(3 hunks)lib/chunkio/src/cio_file.c(1 hunks)src/flb_input_chunk.c(8 hunks)src/flb_input_log.c(2 hunks)src/flb_task.c(4 hunks)tests/internal/CMakeLists.txt(1 hunks)tests/internal/input_chunk_routes.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/internal/input_chunk_routes.c
- tests/internal/CMakeLists.txt
- lib/chunkio/src/cio_file.c
🧰 Additional context used
🧬 Code graph analysis (4)
src/flb_task.c (4)
src/flb_input_chunk.c (4)
flb_input_chunk_has_direct_routes(1298-1321)flb_input_chunk_get_direct_routes(1323-1471)flb_input_chunk_destroy_direct_routes(1473-1491)flb_input_chunk_get_name(2725-2731)include/fluent-bit/flb_mem.h (1)
flb_calloc(84-96)src/flb_router_condition.c (1)
flb_router_chunk_context_destroy(187-190)src/flb_input.c (1)
flb_input_name(790-797)
include/fluent-bit/flb_input_chunk.h (1)
src/flb_input_chunk.c (4)
flb_input_chunk_write_header_v2(1119-1296)flb_input_chunk_has_direct_routes(1298-1321)flb_input_chunk_get_direct_routes(1323-1471)flb_input_chunk_destroy_direct_routes(1473-1491)
src/flb_input_log.c (3)
src/flb_routes_mask.c (1)
flb_routes_mask_get_bit(125-140)include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)src/flb_input_chunk.c (2)
flb_input_chunk_write_header_v2(1119-1296)flb_input_chunk_get_name(2725-2731)
src/flb_input_chunk.c (2)
src/flb_routes_mask.c (2)
flb_routes_mask_set_bit(76-91)flb_routes_mask_set_by_tag(31-67)include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)
🪛 markdownlint-cli2 (0.18.1)
CHUNKS.md
103-103: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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)
- 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 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 - fuzzing test
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=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, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=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_COVERAGE=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_SANITIZE_MEMORY=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 (-DSANITIZE_UNDEFINED=On, 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 (-DSANITIZE_UNDEFINED=On, 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 (-DFLB_JEMALLOC=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, clang, clang++)
- 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, 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-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-compile-without-cxx (3.31.6)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-centos-7
🔇 Additional comments (8)
src/flb_input_log.c (1)
207-289: Direct route persistence implementation looks solid.The two-pass collection approach properly:
- Counts routes before allocation to size the array correctly
- Prefers output alias over name for stable labels
- Bounds-checks label lengths against
UINT16_MAX- Uses
uint16_tfor route IDs, avoiding the truncation issue from past reviews- Includes appropriate warning when persistence fails
- Cleans up allocated memory in all paths
include/fluent-bit/flb_input_chunk.h (1)
28-135: LGTM! Clean API surface for direct route handling.The new declarations properly:
- Use
uint16_tfor route IDs (avoiding truncation)- Include stable textual labels alongside numeric IDs
- Provide forward declaration for
struct cio_chunk- Define consistent flag bits for metadata
- Expose complete lifecycle management (write, check, get, destroy)
src/flb_task.c (2)
75-132: Excellent stable output resolution strategy.The helper properly resolves outputs using a three-tier strategy:
- Match by alias (if label present)
- Match by name (if label present)
- Fallback to numeric ID
This approach ensures chunks continue routing to the intended destination even when output ordering changes between restarts, addressing the concerns from previous reviews about output ID instability.
503-596: Robust direct route restoration with proper validation and fallback.The restoration logic appropriately:
- Pre-validates all stored routes before creating any task routes (lines 510-522)
- Uses atomic semantics: either all routes resolve successfully or none are used
- Falls back gracefully to configured routes when any stored route is missing (lines 567-582)
- Provides actionable warning messages including both numeric ID and textual label (lines 568-574)
- Cleans up allocated resources in all code paths (lines 554-560, 586-600)
- Returns early after successful restoration to avoid duplicate route creation (lines 585-596)
This addresses the previous review concerns about validating stable identifiers and handling config drift.
src/flb_input_chunk.c (4)
65-171: Well-structured metadata view abstraction.The new
flb_input_chunk_meta_viewstructure and parsing function centralize metadata extraction logic, properly:
- Distinguishing legacy format (no magic bytes) from extended format
- Extracting tag with null-terminator detection (lines 136-139)
- Parsing routing payload when direct-routes flag is set (lines 141-168)
- Returning distinct codes for missing magic vs. incomplete routing data (0 vs. 1)
- Avoiding buffer overruns with careful offset arithmetic (lines 154-164)
This provides a clean abstraction for callers like
flb_input_chunk_get_tag.
1119-1296: Solid metadata writer with proper encoding and bounds checking.The v2 header writer correctly:
- Pre-computes label lengths and detects overflow (lines 1171-1195)
- Validates total routing payload fits in
uint16_t(lines 1204-1207)- Truncates tag if necessary to fit in available metadata space (lines 1210-1216)
- Encodes all multi-byte fields in big-endian (lines 1252-1253, 1256-1257, 1261-1262, 1270-1271)
- Sets flags byte appropriately based on label presence (lines 1242-1246)
- Includes null terminator after tag (line 1249)
- Properly cleans up on all error paths (lines 1222-1223, 1287-1289)
1323-1471: Thorough bounds checking in direct route reader.The reader includes comprehensive validation:
- Checks minimum routing payload size before proceeding (lines 1364-1370, 1380-1382)
- Validates label section size when labels flag set (lines 1386-1392)
- Checks each label length against remaining bytes (lines 1441-1445)
- Allocates label buffers with space for null terminator (line 1447)
- Null-terminates all labels (line 1458)
- Returns distinct error codes: -1 for allocation failure, -2 for corrupt data
- Cleans up partial results on any error (lines 1413, 1429, 1443, 1451)
This defensive parsing prevents buffer overruns when reading potentially corrupt or malicious metadata.
956-1038: Proper direct route integration with fallback to tag routing.The loading logic in
flb_input_chunk_mapappropriately:
- Attempts to load direct routes from metadata (line 961)
- Pre-validates all routes resolve to existing outputs (lines 963-975)
- Uses the same stable lookup strategy as task creation (alias → name → id)
- Clears routes_mask and populates it atomically only when all routes valid (lines 977-992)
- Warns with actionable details when any route missing (lines 995-1000, 1004-1006)
- Falls back to tag-based routing when direct routes invalid or produce no routes (lines 1022-1038)
- Cleans up allocated route structures in all paths (lines 1009-1012)
This ensures backlog chunks always route somewhere, even when config has changed.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Signed-off-by: Eduardo Silva <[email protected]>
- New API for direct route persistence - Metadata parsing and writing functions - Backward compatibility maintained Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
edsiper
force-pushed
the
storage-metadata-for-routing
branch
from
2e68457 to
ae8b0c4
Compare
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)
lib/chunkio/src/cio_file_win32.c (1)
49-69: Avoid leavingcf->mapdangling whenCloseHandlefailsAfter
UnmapViewOfFilesucceeds we immediately return on aCloseHandleerror, butcf->mapandcf->alloc_sizeremain unchanged. The view is gone, yet we still report the chunk as “mapped”, so the next teardown will re-unmap an invalid pointer and likely crash. Please null out the map (and zero alloc_size) right afterUnmapViewOfFilesucceeds so the state is safe even if closing the mapping handle fails.- result = UnmapViewOfFile(cf->map); + result = UnmapViewOfFile(cf->map); @@ - result = CloseHandle(cf->backing_mapping); + cf->map = NULL; + cf->alloc_size = 0; + result = CloseHandle(cf->backing_mapping); @@ - cf->backing_mapping = INVALID_HANDLE_VALUE; - cf->alloc_size = 0; - cf->map = NULL; + cf->backing_mapping = INVALID_HANDLE_VALUE;
♻️ Duplicate comments (1)
src/flb_input_chunk.c (1)
715-770: Duplicate: Label mismatch should reject ID-based fallback.This function has the same issue as
task_find_output_referenceinsrc/flb_task.c(lines 76-136): when a label is present but doesn't match any output, it still falls back to ID-only matching, which defeats stable routing. Apply the same fix here.
Duplicate: Extract common output resolution logic.
This is a duplicate of the resolution logic in
src/flb_task.c. See the earlier comment recommending extraction to a shared utility.
🧹 Nitpick comments (6)
lib/chunkio/include/chunkio/cio_file.h (1)
43-54: Consider adding documentation for new fields.The three new fields (
ctx,auto_remap_warned,map_truncated_warned) extend the publiccio_filestruct but lack inline documentation. Consider adding comments to clarify their purpose for users and maintainers.Example:
char *map; /* map of data */ + struct cio_ctx *ctx; /* back-reference to owning context */ - struct cio_ctx *ctx; /* owning context */ #ifdef _WIN32 HANDLE backing_file; HANDLE backing_mapping; #endif int taint_flag; /* content modification flag */ /* cached addr */ char *st_content; crc_t crc_cur; /* crc: current value calculated */ int crc_reset; /* crc: must recalculate from the beginning ? */ + int auto_remap_warned; /* prevents duplicate auto-remap warnings */ + int map_truncated_warned; /* prevents duplicate truncation warnings */ - int auto_remap_warned; /* has sync auto-remap warning been emitted? */ - int map_truncated_warned; /* has RO truncation warning been emitted? */src/flb_task.c (2)
528-570: Remove redundant null check.At lines 536-537, the code checks
if (!stored_output)and continues. However, this block (lines 528-570) is only reached whenstored_routes_valid == FLB_TRUE, which means all outputs were already validated to exist in the loop at lines 514-526. The null check here is dead code.Apply this diff to simplify:
for (direct_output_index = 0; direct_output_index < direct_output_count; direct_output_index++) { stored_output = task_find_output_reference(config, &direct_routes[direct_output_index]); - if (!stored_output) { - continue; - } route = flb_calloc(1, sizeof(struct flb_task_route));
572-578: Log format string could expose label buffer overrun.The warning message at lines 572-578 uses
%.*swithmissing_output_label_lengthto print the label. However, ifmissing_output_labelis non-NULL but not properly null-terminated andmissing_output_label_lengthis incorrectly larger than the actual allocation, this could read past the buffer. While the parsing code should prevent this, defensive programming suggests either ensuring the label is null-terminated or clamping the length.Consider adding a bounds check:
+ int safe_label_len = (missing_output_label && missing_output_label_length > 0) + ? (int) missing_output_label_length : 0; flb_warn("[task] input=%s/%s stored direct route id=%u label=%.*s not found for chunk %s, falling back to configured routes", i_ins->p->name, flb_input_name(i_ins), (unsigned int) missing_output_id, - (int) missing_output_label_length, + safe_label_len, missing_output_label ? missing_output_label : "", flb_input_chunk_get_name(task_ic));src/flb_input_chunk.c (3)
92-171: Document non-zero success return values.The function
input_chunk_metadata_viewreturns three different values:-1for read errors,0for success, and1for malformed routing metadata (lines 150, 157, 164). However, callers likeflb_input_chunk_has_direct_routes(line 1334) andflb_input_chunk_get_direct_routes(line 1396) only check for-1and treat any non-negative value as success. Return value1indicates corrupted metadata that should be handled differently than clean success. Consider either documenting this tri-state return or refactoring callers to distinguish corruption from success.Example documentation:
/* * Parse chunk metadata into a view structure. * * Returns: * 0 : Success, metadata parsed correctly * 1 : Metadata present but routing data is malformed/incomplete * -1 : Fatal error reading metadata */ static int input_chunk_metadata_view(struct flb_input_chunk *ic, struct flb_input_chunk_meta_view *view)
1121-1327: LGTM with minor clarification suggestion.The header writing logic is thorough and correctly handles variable-width IDs, optional labels, and big-endian encoding. Bounds checking prevents metadata from exceeding 64KB limit.
Optional: Strengthen label write guard.
At lines 1308-1310, the code checks
routes[index].label != NULLbefore copying label data, butresolved_lengths[index]could theoretically be non-zero even if label is NULL (though the pre-scan should prevent this). For extra clarity, consider:label_length = resolved_lengths[index]; - if (label_length > 0 && routes[index].label != NULL) { + if (label_length > 0) { + /* resolved_lengths is only set non-zero when label was non-NULL */ memcpy(meta + label_offset, routes[index].label, label_length); }
958-1040: Direct route validation logic matches task creation.The direct routing restoration in
flb_input_chunk_mapcorrectly mirrors the validation and fallback logic inflb_task_create. Both validate that all persisted routes exist before applying them, and both fall back to tag-based routing with appropriate warnings when direct routes are invalid or missing.Clarify warning when direct routes produce zero routes.
The warning at lines 1032-1034 ("direct routing metadata for chunk %s produced no routes") could be misunderstood. It's not that the metadata was invalid (which is caught at line 1006), but that all direct routes were successfully loaded yet none matched current outputs, or all were filtered out by the routes_mask. Consider making this clearer:
else if (has_routes == 0) { flb_plg_warn(in, - "direct routing metadata for chunk %s produced no routes, falling back to tag routing", + "direct routes for chunk %s loaded but resulted in zero active routes, falling back to tag routing", flb_input_chunk_get_name(ic));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
CHUNKS.md(1 hunks)include/fluent-bit/flb_input_chunk.h(3 hunks)lib/chunkio/.github/workflows/ci.yaml(1 hunks)lib/chunkio/CMakeLists.txt(1 hunks)lib/chunkio/include/chunkio/cio_file.h(2 hunks)lib/chunkio/src/cio_file.c(11 hunks)lib/chunkio/src/cio_file_unix.c(1 hunks)lib/chunkio/src/cio_file_win32.c(7 hunks)lib/chunkio/tests/CMakeLists.txt(2 hunks)lib/chunkio/tests/fs.c(2 hunks)lib/chunkio/tests/fs_windows.c(1 hunks)lib/chunkio/tests/metadata_update.c(1 hunks)src/flb_input_chunk.c(8 hunks)src/flb_input_log.c(2 hunks)src/flb_task.c(5 hunks)tests/internal/CMakeLists.txt(1 hunks)tests/internal/input_chunk_routes.c(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- lib/chunkio/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- include/fluent-bit/flb_input_chunk.h
🧰 Additional context used
🧠 Learnings (7)
📚 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:
src/flb_task.c
📚 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:
src/flb_task.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:
src/flb_task.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:
src/flb_task.c
📚 Learning: 2025-08-29T06:24:44.797Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:26-26
Timestamp: 2025-08-29T06:24:44.797Z
Learning: In Fluent Bit, ZSTD support is always available and enabled by default. The build system automatically detects and uses either the system libzstd library or builds the bundled ZSTD version. Unlike other optional dependencies like Arrow which use conditional compilation guards (e.g., FLB_HAVE_ARROW), ZSTD does not require conditional includes or build flags.
Applied to files:
src/flb_task.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:
src/flb_task.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:
src/flb_task.c
🧬 Code graph analysis (9)
lib/chunkio/tests/fs.c (2)
lib/chunkio/tests/fs_windows.c (1)
log_cb(59-67)lib/chunkio/tests/metadata_update.c (1)
log_cb(34-44)
lib/chunkio/tests/fs_windows.c (3)
lib/chunkio/src/cio_file_unix.c (6)
cio_file_native_delete(457-475)cio_file_native_get_size(317-341)cio_file_native_open(396-417)cio_file_native_map(74-111)cio_file_native_unmap(47-72)cio_file_native_close(419-440)lib/chunkio/src/cio_file_win32.c (6)
cio_file_native_delete(514-559)cio_file_native_get_size(366-381)cio_file_native_open(436-474)cio_file_native_map(73-161)cio_file_native_unmap(33-71)cio_file_native_close(476-497)lib/chunkio/src/cio_file.c (2)
cio_file_down(920-959)cio_file_sync(1147-1250)
lib/chunkio/src/cio_file.c (2)
lib/chunkio/src/cio_file_unix.c (2)
cio_file_native_unmap(47-72)cio_file_native_close(419-440)lib/chunkio/src/cio_file_win32.c (2)
cio_file_native_unmap(33-71)cio_file_native_close(476-497)
src/flb_input_log.c (3)
src/flb_routes_mask.c (1)
flb_routes_mask_get_bit(125-140)include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)src/flb_input_chunk.c (2)
flb_input_chunk_write_header_v2(1121-1327)flb_input_chunk_get_name(2780-2786)
src/flb_task.c (4)
src/flb_input_chunk.c (4)
flb_input_chunk_has_direct_routes(1329-1352)flb_input_chunk_get_direct_routes(1354-1526)flb_input_chunk_destroy_direct_routes(1528-1546)flb_input_chunk_get_name(2780-2786)include/fluent-bit/flb_mem.h (1)
flb_calloc(84-96)src/flb_router_condition.c (1)
flb_router_chunk_context_destroy(187-190)src/flb_input.c (1)
flb_input_name(790-797)
lib/chunkio/src/cio_file_win32.c (1)
lib/chunkio/src/cio_file_unix.c (4)
cio_file_native_get_size(317-341)cio_file_native_resize(499-571)cio_file_native_unmap(47-72)cio_file_native_close(419-440)
lib/chunkio/tests/metadata_update.c (2)
lib/chunkio/tests/fs.c (1)
log_cb(45-52)lib/chunkio/tests/fs_windows.c (1)
log_cb(59-67)
tests/internal/input_chunk_routes.c (2)
include/fluent-bit/flb_mem.h (1)
flb_free(126-128)src/flb_input_chunk.c (5)
flb_input_chunk_write_header_v2(1121-1327)flb_input_chunk_has_direct_routes(1329-1352)flb_input_chunk_get_direct_routes(1354-1526)flb_input_chunk_destroy_direct_routes(1528-1546)flb_input_chunk_get_tag(2830-2847)
src/flb_input_chunk.c (2)
src/flb_routes_mask.c (2)
flb_routes_mask_set_bit(76-91)flb_routes_mask_set_by_tag(31-67)include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)
🪛 markdownlint-cli2 (0.18.1)
CHUNKS.md
105-105: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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)
- 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: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=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_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 (-DSANITIZE_UNDEFINED=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_COVERAGE=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=Off, 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 (-DSANITIZE_UNDEFINED=On, 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 (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- 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=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 (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: PR - fuzzing test
- 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-without-cxx (3.31.6)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-centos-7
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
🔇 Additional comments (20)
lib/chunkio/tests/fs.c (2)
967-1102: LGTM! Well-structured test for unsigned underflow scenario.The test effectively validates the fix for the unsigned underflow bug by creating a minimal chunk, writing small metadata and content, then writing larger metadata that would trigger the underflow in the old code. The test properly verifies data integrity through the full lifecycle (write, sync, down/up) with appropriate cleanup.
1116-1116: LGTM! Test properly registered.The new test is correctly added to the TEST_LIST.
lib/chunkio/tests/metadata_update.c (3)
55-185: LGTM! Comprehensive test for metadata update with content preservation.The test effectively validates that content data is correctly preserved when metadata is updated to a larger size. The test properly exercises the memmove fix mentioned in the comments and verifies data integrity through the full persistence cycle.
191-282: LGTM! Effective test for multiple metadata updates.The test validates that content remains intact across multiple metadata updates of varying sizes, including both growing and shrinking scenarios. Memory management is correct with proper cleanup in the loop.
284-288: LGTM! Tests properly registered.Both test functions are correctly added to the TEST_LIST.
lib/chunkio/tests/CMakeLists.txt (2)
12-21: LGTM! Test files properly added to build system.The new test files
metadata_update.candfs_windows.care correctly added with appropriate conditional compilation guards for Windows-specific tests.
47-51: LGTM! Performance test added correctly.The
fs_fragmentation.cperformance test is properly added to UNIT_PERF_TESTS with clear documentation that these tests require manual execution.tests/internal/CMakeLists.txt (1)
29-29: LGTM! Test file properly added.The new test file
input_chunk_routes.cis correctly added to UNIT_TESTS_FILES.lib/chunkio/src/cio_file_unix.c (1)
69-69: LGTM! Warning flag properly reset on unmap.Resetting
map_truncated_warnedtoCIO_FALSEwhen unmapping is appropriate, as it allows the warning to be emitted again if the chunk is remapped later and the issue recurs. This maintains consistency with the reset of other state variables.lib/chunkio/.github/workflows/ci.yaml (2)
21-31: LGTM! Windows CI modernized with CMake and MSVC.The changes to use CMake with NMake Makefiles and set up the MSVC developer command prompt are appropriate improvements over the custom batch script approach.
18-18: No issues found.The
windows-2025runner image is available in GitHub Actions and was made generally available in April 2025. The current code is correct and requires no changes.CHUNKS.md (1)
94-137: LGTM! Comprehensive documentation of direct route persistence.The documentation clearly explains the new routing metadata format, feature flags, and backward compatibility considerations. The diagrams effectively illustrate the structure of the routing payload.
src/flb_input_log.c (1)
252-277: Widened direct-route IDs look goodStoring
route_path->ins->idin auint32_troute buffer eliminates the earlier 8-bit truncation risk and keeps metadata aligned with the v2 header. Appreciate the clean two-pass build-up before writing the header.lib/chunkio/tests/fs_windows.c (1)
75-360: Great Windows regression coverageThese targeted cases (auto-unmap on delete, sync while down, map-size mismatch) give us confidence the platform-specific fixes behave as intended.
tests/internal/input_chunk_routes.c (1)
132-189: Nice wide-ID direct-route round trip testExercising both 16-bit and >16-bit output IDs (511 and 70000) with label payloads ensures the new metadata writer/reader paths handle wide IDs and strings correctly.
src/flb_task.c (2)
22-22: LGTM!The header additions are appropriate for the new direct routing functionality (integer types and string operations).
Also applies to: 34-34
553-565: LGTM!The cleanup path on allocation failure is thorough: router context is destroyed, direct routes are freed, and the task is properly torn down. Setting
event_chunk->data = NULLbefore destroy prevents double-free since the buffer is still owned by the input chunk.src/flb_input_chunk.c (3)
42-43: LGTM!Header additions are appropriate for the new direct routing functionality.
1354-1526: LGTM!The direct routes parsing is robust with thorough validation at each step: size checks prevent buffer overruns, allocation failures are handled cleanly with proper cleanup, and labels are correctly null-terminated for C string usage.
2830-2847: LGTM!Refactoring
flb_input_chunk_get_tagto use the metadata view infrastructure is consistent and maintains the same error handling behavior while consolidating metadata parsing logic.
| ``` | ||
| Metadata | ||
| -- +---------+-------+ | ||
| / | 0xF1 | 0x77 | <- Magic Bytes | ||
| / +---------+-------+ | ||
| < | Type | Flags | <- Chunk type and flag bits | ||
| \ +---------+-------+ | ||
| \ | Tag | <- Tag string (no size prefix) | ||
| +---------+-------+ | ||
| | 0x00 | | <- Tag terminator (present when flags != 0) | ||
| +---------+-------+ | ||
| | Routing Length | <- uint16_t big endian | ||
| +---------+-------+ | ||
| | Route Count | <- uint16_t big endian | ||
| +---------+-------+ | ||
| | Output IDs ... | <- Each stored as uint16_t (big endian) | ||
| | | or uint32_t when FLB_CHUNK_FLAG_DIRECT_ROUTE_WIDE_IDS | ||
| +---------+-------+ | ||
| | Label Lens ... | <- Present when FLB_CHUNK_FLAG_DIRECT_ROUTE_LABELS | ||
| +---------+-------+ | ||
| | Label Bytes ... | <- Concatenated label strings (no terminators) | ||
| -- +-----------------+ | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to fenced code block.
The fenced code block at line 105 should specify a language identifier for proper rendering and syntax highlighting.
Apply this fix:
-```
+```text
Metadata
-- +---------+-------+Based on static analysis findings.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
105-105: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In CHUNKS.md around lines 105 to 127 the fenced code block lacks a language
identifier; replace the opening triple backticks with a language-marked fence
(e.g., change ``` to ```text) so the block starts with ```text and keep the rest
of the block and closing fence unchanged to enable proper rendering and
highlighting.
| static struct flb_output_instance *task_find_output_reference(struct flb_config *config, | ||
| const struct flb_chunk_direct_route *route) | ||
| { | ||
| int alias_length; | ||
| int label_length; | ||
| int name_length; | ||
| const char *label; | ||
| uint32_t stored_id; | ||
| struct mk_list *head; | ||
| struct flb_output_instance *o_ins; | ||
|
|
||
| alias_length = 0; | ||
| label_length = 0; | ||
| name_length = 0; | ||
| label = NULL; | ||
| stored_id = 0; | ||
|
|
||
| if (!config || !route) { | ||
| return NULL; | ||
| } | ||
|
|
||
| label = route->label; | ||
| stored_id = route->id; | ||
| if (label != NULL) { | ||
| label_length = route->label_length; | ||
| if (label_length == 0) { | ||
| label_length = (int) strlen(label); | ||
| } | ||
| } | ||
|
|
||
| if (label != NULL && label_length > 0) { | ||
| mk_list_foreach(head, &config->outputs) { | ||
| o_ins = mk_list_entry(head, struct flb_output_instance, _head); | ||
| if (o_ins->alias != NULL) { | ||
| alias_length = (int) strlen(o_ins->alias); | ||
| if (alias_length == label_length && | ||
| strncmp(o_ins->alias, label, (size_t) label_length) == 0) { | ||
| return o_ins; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| mk_list_foreach(head, &config->outputs) { | ||
| o_ins = mk_list_entry(head, struct flb_output_instance, _head); | ||
| name_length = (int) strlen(o_ins->name); | ||
| if (name_length == label_length && | ||
| strncmp(o_ins->name, label, (size_t) label_length) == 0) { | ||
| return o_ins; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| mk_list_foreach(head, &config->outputs) { | ||
| o_ins = mk_list_entry(head, struct flb_output_instance, _head); | ||
| if ((uint32_t) o_ins->id == stored_id) { | ||
| return o_ins; | ||
| } | ||
| } | ||
|
|
||
| return NULL; | ||
| } |
There was a problem hiding this comment.
Label mismatch should reject ID-based fallback.
The resolution logic searches by label (alias/name), then unconditionally falls back to numeric ID regardless of whether a label was present. This means a persisted route with label="old_output" and id=5 will still match an output at id=5 even if its current label is "new_output", defeating the purpose of stable identifiers. When a label is present but no match is found by label, the function should return NULL instead of falling back to ID-only matching. The ID-only path should be reserved for legacy chunks that lack labels.
Apply this fix to require label match when label is present:
}
+ /* If label was present but didn't match, reject ID-only fallback */
+ if (label != NULL && label_length > 0) {
+ return NULL;
+ }
+
mk_list_foreach(head, &config->outputs) {
o_ins = mk_list_entry(head, struct flb_output_instance, _head);
if ((uint32_t) o_ins->id == stored_id) {Refactor: Extract common output resolution logic.
This function duplicates the same resolution logic as input_chunk_find_output_reference in src/flb_input_chunk.c (lines 715-770). Both implementations are nearly identical. Consider extracting this logic into a shared utility function in a common module (e.g., flb_router.c or flb_output.c) to eliminate duplication and ensure consistent behavior.
🤖 Prompt for AI Agents
In src/flb_task.c around lines 76-136, the function currently falls back to
matching by numeric ID even when a label was provided but not matched; change
the control flow so that if route->label is non-NULL (label_length > 0) and no
alias/name match is found, the function returns NULL instead of continuing to
ID-based lookup; additionally, refactor by extracting the common resolution
logic (label/alias/name/ID resolution) into a shared utility (e.g., new
flb_router_lookup_output or similar in a common module) so both this function
and input_chunk_find_output_reference in src/flb_input_chunk.c call the same
implementation to avoid duplication and ensure consistent behavior.
Signed-off-by: Eduardo Silva <[email protected]>
Signed-off-by: Eduardo Silva <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/flb_input_chunk.c(8 hunks)src/flb_task.c(5 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 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:
src/flb_task.c
📚 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:
src/flb_task.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:
src/flb_task.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:
src/flb_task.c
📚 Learning: 2025-08-29T06:24:44.797Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:26-26
Timestamp: 2025-08-29T06:24:44.797Z
Learning: In Fluent Bit, ZSTD support is always available and enabled by default. The build system automatically detects and uses either the system libzstd library or builds the bundled ZSTD version. Unlike other optional dependencies like Arrow which use conditional compilation guards (e.g., FLB_HAVE_ARROW), ZSTD does not require conditional includes or build flags.
Applied to files:
src/flb_task.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:
src/flb_task.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:
src/flb_task.c
🧬 Code graph analysis (2)
src/flb_task.c (4)
src/flb_input_chunk.c (4)
flb_input_chunk_has_direct_routes(1332-1355)flb_input_chunk_get_direct_routes(1357-1529)flb_input_chunk_destroy_direct_routes(1531-1549)flb_input_chunk_get_name(2783-2789)include/fluent-bit/flb_mem.h (1)
flb_calloc(84-96)src/flb_router_condition.c (1)
flb_router_chunk_context_destroy(187-190)src/flb_input.c (1)
flb_input_name(790-797)
src/flb_input_chunk.c (2)
src/flb_routes_mask.c (2)
flb_routes_mask_set_bit(76-91)flb_routes_mask_set_by_tag(31-67)include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)
⏰ 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)
- 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-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- 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: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=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_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: pr-compile-centos-7
- 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, clang, clang++)
- GitHub Check: pr-compile-without-cxx (3.31.6)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=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_ARROW=On, 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 (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=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, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=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_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=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 (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: PR - fuzzing test
| meta[0] = FLB_INPUT_CHUNK_MAGIC_BYTE_0; | ||
| meta[1] = FLB_INPUT_CHUNK_MAGIC_BYTE_1; | ||
|
|
||
| if (event_type == FLB_INPUT_LOGS) { | ||
| meta[2] = FLB_INPUT_CHUNK_TYPE_LOGS; | ||
| } | ||
| else if (event_type == FLB_INPUT_METRICS) { | ||
| meta[2] = FLB_INPUT_CHUNK_TYPE_METRICS; | ||
| } | ||
| else if (event_type == FLB_INPUT_TRACES) { | ||
| meta[2] = FLB_INPUT_CHUNK_TYPE_TRACES; | ||
| } | ||
| else if (event_type == FLB_INPUT_PROFILES) { | ||
| meta[2] = FLB_INPUT_CHUNK_TYPE_PROFILES; | ||
| } | ||
|
|
||
| flags = FLB_CHUNK_FLAG_DIRECT_ROUTES; | ||
| if (has_labels == FLB_TRUE) { | ||
| flags |= FLB_CHUNK_FLAG_DIRECT_ROUTE_LABELS; | ||
| } | ||
| if (wide_ids == FLB_TRUE) { | ||
| flags |= FLB_CHUNK_FLAG_DIRECT_ROUTE_WIDE_IDS; | ||
| } | ||
| meta[3] = (char) flags; | ||
|
|
||
| memcpy(meta + FLB_INPUT_CHUNK_META_HEADER, tag, tag_len); | ||
| meta[FLB_INPUT_CHUNK_META_HEADER + tag_len] = '\0'; | ||
|
|
||
| offset = FLB_INPUT_CHUNK_META_HEADER + tag_len + 1; | ||
| meta[offset] = (uint8_t) (routing_length >> 8); | ||
| meta[offset + 1] = (uint8_t) (routing_length & 0xFF); | ||
|
|
||
| stored_count = (uint16_t) route_count; | ||
| meta[offset + 2] = (uint8_t) (stored_count >> 8); | ||
| meta[offset + 3] = (uint8_t) (stored_count & 0xFF); | ||
|
|
||
| id_offset = offset + 4; | ||
| for (index = 0; index < route_count; index++) { | ||
| if (wide_ids == FLB_TRUE) { | ||
| meta[id_offset] = (uint8_t) ((routes[index].id >> 24) & 0xFF); | ||
| meta[id_offset + 1] = (uint8_t) ((routes[index].id >> 16) & 0xFF); | ||
| meta[id_offset + 2] = (uint8_t) ((routes[index].id >> 8) & 0xFF); | ||
| meta[id_offset + 3] = (uint8_t) (routes[index].id & 0xFF); | ||
| } | ||
| else { | ||
| meta[id_offset] = (uint8_t) ((routes[index].id >> 8) & 0xFF); | ||
| meta[id_offset + 1] = (uint8_t) (routes[index].id & 0xFF); | ||
| } | ||
| id_offset += id_bytes; | ||
| } | ||
|
|
||
| if (has_labels == FLB_TRUE) { | ||
| label_offset = offset + 4 + (route_count * id_bytes); | ||
| for (index = 0; index < route_count; index++) { | ||
| label_length = resolved_lengths[index]; | ||
| meta[label_offset] = (uint8_t) (label_length >> 8); | ||
| meta[label_offset + 1] = (uint8_t) (label_length & 0xFF); | ||
| label_offset += sizeof(uint16_t); | ||
| } | ||
|
|
||
| for (index = 0; index < route_count; index++) { | ||
| label_length = resolved_lengths[index]; | ||
| if (label_length > 0 && routes[index].label != NULL) { | ||
| memcpy(meta + label_offset, routes[index].label, label_length); | ||
| } | ||
| label_offset += label_length; | ||
| } | ||
| } | ||
|
|
||
| ret = cio_meta_write(chunk, (char *) meta, meta_size); | ||
| if (ret == -1) { | ||
| flb_error("[input chunk] could not write metadata"); | ||
| flb_free(resolved_lengths); | ||
| flb_free(meta); | ||
| return -1; | ||
| } | ||
|
|
||
| flb_free(resolved_lengths); | ||
| flb_free(meta); | ||
|
|
There was a problem hiding this comment.
Persist plugin identity alongside IDs/labels
flb_input_chunk_write_header_v2() only serializes the numeric id and optional label for each direct route. Because task_find_output_reference() (and the input-side restore helper) accept any output whose alias/name matches that label, a chunk recorded against an http output can be resurrected to a completely different plugin if the operator later reuses the alias for, say, an elasticsearch output. To make restoration safe, we need to encode a stable plugin identifier in the routing payload (for example the plugin type name or a hash of the config) and teach both the reader and the lookup helpers to validate it before accepting a match. Without that extra field we still risk sending backlog data to the wrong destination after common reconfiguration changes.
| if (label != NULL && label_length > 0) { | ||
| mk_list_foreach(head, &config->outputs) { | ||
| o_ins = mk_list_entry(head, struct flb_output_instance, _head); | ||
| if (o_ins->alias != NULL) { | ||
| alias_length = (int) strlen(o_ins->alias); | ||
| if (alias_length == label_length && | ||
| strncmp(o_ins->alias, label, (size_t) label_length) == 0) { | ||
| return o_ins; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| mk_list_foreach(head, &config->outputs) { | ||
| o_ins = mk_list_entry(head, struct flb_output_instance, _head); | ||
| name_length = (int) strlen(o_ins->name); | ||
| if (name_length == label_length && | ||
| strncmp(o_ins->name, label, (size_t) label_length) == 0) { | ||
| return o_ins; | ||
| } | ||
| } | ||
|
|
||
| /* If label was present but didn't match, reject ID-only fallback */ | ||
| return NULL; | ||
| } | ||
|
|
||
| mk_list_foreach(head, &config->outputs) { | ||
| o_ins = mk_list_entry(head, struct flb_output_instance, _head); | ||
| if ((uint32_t) o_ins->id == stored_id) { | ||
| return o_ins; |
There was a problem hiding this comment.
Ensure direct-route restoration validates plugin type
We now reject ID-only fallback when a label is present, but the lookup still returns the first output whose alias/name matches the stored label without confirming that it is the same plugin type as the one that originally emitted the chunk. If an operator reuses the alias for a different output (or swaps the plugin type behind the same name) between restarts, a backlog chunk will happily bind to the new destination and ship data to the wrong system again—the exact misrouting scenario we were trying to prevent when we added stable identifiers.
Please extend the stored metadata and the lookup so we also persist a stable plugin identifier (e.g., o_ins->p->name plus maybe a hash/version) and verify it before accepting a match. If the plugin type no longer matches, treat the route as missing and fall back to standard routing.
Work in process.
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
Documentation
Improvements
Tests