router_config: wire every route merging into a shared output#12142
router_config: wire every route merging into a shared output#12142zshuang0316 wants to merge 3 commits into
Conversation
f1582ad to
a94ba37
Compare
|
@codex review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughDirect-route detection now matches route/output pairs, while task creation deduplicates routes per output instance. A regression test confirms that distinct routes sharing an output retain separate direct paths. ChangesRouter direct route handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/internal/router_config.c (1)
1342-1357: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover identical route-output deduplication.
This verifies distinct routes sharing an output, but not the identical-pair branch. Add another
printmeoutput edge toroute_oneand keep the expected direct-path count at two.As per coding guidelines, “Add or update tests for behavior changes.”
🤖 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/internal/router_config.c` around lines 1342 - 1357, Add a second output edge using route_output_one’s “printme” destination to route_one, exercising identical route-output deduplication while preserving the expected direct-path count of two in the relevant router configuration test.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@tests/internal/router_config.c`:
- Around line 1342-1357: Add a second output edge using route_output_one’s
“printme” destination to route_one, exercising identical route-output
deduplication while preserving the expected direct-path count of two in the
relevant router configuration test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ae50a7ba-14f8-46b1-89d1-64fa8be902cf
📒 Files selected for processing (2)
src/flb_router_config.ctests/internal/router_config.c
![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: a94ba37d9a
ℹ️ 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".
| cfl_list_foreach(head, &in->routes_direct) { | ||
| path = cfl_list_entry(head, struct flb_router_path, _head); | ||
| if (path->ins == out) { | ||
| if (path->ins == out && path->route == route) { |
There was a problem hiding this comment.
When two conditional routes target the same output and their predicates can both match a record, this route-aware dedupe leaves both flb_router_paths in routes_direct, but the later routing mask is only keyed by output id. A route-specific chunk for either route therefore passes both paths in flb_task_create() and creates duplicate flb_task_route entries for the same output; the task status/data helpers then update only the first matching route->out, so delivery, retry, and accounting for that chunk can be duplicated or inconsistent. Please keep the per-route paths for condition evaluation, but collapse matched paths to one task route per output or carry route identity through the mask/task layer.
AGENTS.md reference: AGENTS.md:L224-L228
Useful? React with 👍 / 👎.
flb_router_apply_config() deduplicated direct routes by output instance only (input_has_direct_route). When several routes point at the same output instance, only the first was connected and the rest were silently dropped, even though each route carries its own condition and is evaluated independently at runtime through its own flb_router_path. Key the deduplication on the (route, output) pair so distinct routes merging into one output are each wired as their own direct path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: zshuang0316 <zshuang0316@163.com>
Routes are now deduplicated per (route, output) pair, so several matching routes can resolve to the same output instance. flb_task_create() builds one flb_task_route per direct path, so a chunk matching more than one such route produced duplicate task routes for the same output. The engine dispatcher iterates task routes and flushes each one, so the chunk was delivered to that output more than once and its records/bytes were counted multiple times. Skip outputs that already have a task route when building both the live and restored direct-route task paths, keeping at most one route per output. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: zshuang0316 <zshuang0316@163.com>
Add a router_config case asserting that two distinct routes targeting the same output instance each produce their own direct path, while a single route listing that output twice collapses to one path. This guards the (route, output) deduplication against both dropping distinct routes and duplicating identical ones. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: zshuang0316 <zshuang0316@163.com>
a94ba37 to
7188d83
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7188d83ebb
ℹ️ 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".
| cfl_list_foreach(head, &in->routes_direct) { | ||
| path = cfl_list_entry(head, struct flb_router_path, _head); | ||
| if (path->ins == out) { | ||
| if (path->ins == out && path->route == route) { |
There was a problem hiding this comment.
With filesystem storage and total_limit_size, allowing multiple routes_direct entries for the same output breaks the existing route-mask accounting path because masks are keyed only by output id. In route_payload_apply_outputs(), a conditional/per-record payload chunk that was already fs_counted (for example when the output also has Match *) subtracts and clears any “other route” whose output bit is set; after this change, a second route to the same output matches that test, so the current output is subtracted even though the payload still routes there, and it is not added back because chunk->fs_counted remains true. This undercounts the output’s fs_chunks_size and can bypass storage pressure for shared-output conditional routes.
AGENTS.md reference: AGENTS.md:L239-L241
Useful? React with 👍 / 👎.
1 participant
Summary
flb_router_apply_config()deduplicated direct routes by output instance only(
input_has_direct_route()). When several routes point at the same outputinstance, only the first was connected via
flb_router_connect_direct()and therest were silently dropped.
This is incorrect because each route carries its own
conditionand is evaluatedindependently at runtime through its own
flb_router_path— seeflb_input_log.c,where per-record routing payloads are keyed by
route_path->routeand each route'scondition is evaluated separately (
flb_router_condition_evaluate_record(payload->route, ...)).Collapsing multiple routes that merge into a shared output to a single path means
records that should reach that output via a dropped route's condition never get
routed there — the intended union across routes is broken.
Example
Before this change only
error_logsis wired toes; records matchingcheckout_logs(but noterror_logs) never reaches.Fix
Key the deduplication on the
(route, output)pair so distinct routes merginginto one output are each wired as their own direct path. Identical
(route, output)edges (e.g. an output listed twice within one route) are still collapsed.
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:
Added an internal regression test (
tests/internal/router_config.c,apply_config_merged_output_keeps_all_routes) asserting that two distinct routestargeting the same output instance each produce their own
flb_router_path.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.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests