out_s3, out_chronicle: grow log_key buffer to avoid dropping oversized values - #12137
out_s3, out_chronicle: grow log_key buffer to avoid dropping oversized values#12137vitaly-sinev wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughChronicle and S3 now resize scratch buffers and retry JSON serialization when log-key values exceed the initial capacity. Allocation failures free the buffer and abort extraction. ChangesLog-key serialization
Estimated code review effort: 2 (Simple) | ~10 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugins/out_chronicle/chronicle.c (1)
604-623: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply the required control-block brace style consistently.
plugins/out_chronicle/chronicle.c#L604-L623: move opening braces for the newwhileandifblocks onto the following line.plugins/out_s3/s3.c#L3695-L3715: make the same change for the new retry-loop blocks.🤖 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/out_chronicle/chronicle.c` around lines 604 - 623, Apply the project’s brace style to the new control blocks: in plugins/out_chronicle/chronicle.c lines 604-623, update the while and if blocks; in plugins/out_s3/s3.c lines 3695-3715, make the same adjustment for each new retry-loop block. Move each opening brace onto the following line without changing control flow.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.
Inline comments:
In `@plugins/out_s3/s3.c`:
- Around line 3707-3715: Update the allocation-failure handling in the decoder
path around alloc_error so any failure after records have been appended cannot
return a partial payload. After destroying the decoder, free val_buf and return
NULL whenever alloc_error is set, regardless of val_offset or previously decoded
data.
---
Nitpick comments:
In `@plugins/out_chronicle/chronicle.c`:
- Around line 604-623: Apply the project’s brace style to the new control
blocks: in plugins/out_chronicle/chronicle.c lines 604-623, update the while and
if blocks; in plugins/out_s3/s3.c lines 3695-3715, make the same adjustment for
each new retry-loop block. Move each opening brace onto the following line
without changing control flow.
🪄 Autofix (Beta)
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
Run ID: 67c22f54-452d-47a4-a28e-2dad486fe111
📒 Files selected for processing (2)
plugins/out_chronicle/chronicle.cplugins/out_s3/s3.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: f25bf7808d
ℹ️ 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".
| alloc_error = 1; | ||
| break; |
There was a problem hiding this comment.
For S3 chunks using log_key, if a later non-string value needs this buffer growth and flb_realloc() fails after earlier records have already advanced val_offset, setting alloc_error only exits the decoder loop; the function then falls through and creates an SDS from the partial val_buf whenever val_offset != 0. The flush path treats that non-NULL chunk as data to upload, so the current and remaining records can be dropped while the output reports success; free the buffer and return NULL on this allocation failure instead of falling through with partial data.
Useful? React with 👍 / 👎.
f25bf78 to
032f864
Compare
|
Good catch (both @coderabbitai and @chatgpt-codex-connector) — fixed: on |
|
To use Codex here, create a Codex account and connect to github. |
|
Tip For best results, initiate chat on the files or code changes.
🐇✅ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
The log_key extraction buffer was sized bytes + bytes/4 and never grown. A non-string log_key value whose JSON form exceeded the remaining space made flb_msgpack_to_json() fail; the code only broke out of the loop, so the oversized value was silently dropped (and, before that, could be emitted truncated). Grow the scratch buffer and retry the conversion until the value fits, so the value is neither truncated nor dropped. The guard on the remaining size also avoids calling flb_msgpack_to_json() with a zero-length buffer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: v.sinev <vit.phantom@gmail.com>
The log_key extraction buffer was sized bytes + bytes/4 and never grown. A non-string log_key value whose JSON form exceeded the remaining space made flb_msgpack_to_json() fail; the code only broke out of the loop, so the oversized value was silently dropped. Grow the scratch buffer and retry the conversion until the value fits, so the value is neither truncated nor dropped. The guard on the remaining size also avoids calling flb_msgpack_to_json() with a zero-length buffer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: v.sinev <vit.phantom@gmail.com>
032f864 to
c476849
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
plugins/out_s3/s3.c (3)
4504-4506: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the configuration descriptions.
The format parser accepts
arrow, but the format description omits it. The compression parser acceptsnone, whilearrowand deprecatedparquetselect formats rather than compression codecs. Separate these legacy aliases from the codec list.Also applies to: 4577-4580
🤖 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/out_s3/s3.c` around lines 4504 - 4506, Update the format and compression configuration descriptions near the output-format options to list arrow as a supported format, and describe compression as accepting none, snappy, zstd, and gzip codecs. Keep arrow and deprecated parquet identified as format aliases rather than compression codecs, and state the default consistently.
1020-1047: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject conflicting format selectors.
When
format=arrowandcompression=parquetare both configured, these branches callenable_parquet_format()and replace the explicit format. The reverse combination has the same problem. Reject mismatches, or give the explicitformatproperty precedence.🤖 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/out_s3/s3.c` around lines 1020 - 1047, Update the format-selection logic around enable_parquet_format() and enable_arrow_format() to detect when compression-based selectors conflict with an explicitly configured format. Reject mismatches with an error, or ensure the explicit format takes precedence; preserve valid matching configurations and existing deprecation handling.
1785-1807: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRetry backlog chunks after conversion failure.
The live path returns
FLB_RETRY, but this path continues tos3_put_object()after columnar conversion fails. It can store JSON under a Parquet or Arrow format. If generic compression fails, it can also send an uncompressed body whilecreate_headers()still advertisesContent-Encoding. Return failure and retain the chunk for retry, or update the per-request format and headers before fallback.As per coding guidelines, verify backlog/live-ingestion parity.
🤖 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/out_s3/s3.c` around lines 1785 - 1807, Update the failure handling in the columnar conversion and generic compression branches before s3_put_object(): match the live path by returning FLB_RETRY and retaining the backlog chunk when conversion or compression fails, rather than uploading fallback data with stale format or Content-Encoding headers. Verify the backlog path uses the same retry behavior as live ingestion.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.
Outside diff comments:
In `@plugins/out_s3/s3.c`:
- Around line 4504-4506: Update the format and compression configuration
descriptions near the output-format options to list arrow as a supported format,
and describe compression as accepting none, snappy, zstd, and gzip codecs. Keep
arrow and deprecated parquet identified as format aliases rather than
compression codecs, and state the default consistently.
- Around line 1020-1047: Update the format-selection logic around
enable_parquet_format() and enable_arrow_format() to detect when
compression-based selectors conflict with an explicitly configured format.
Reject mismatches with an error, or ensure the explicit format takes precedence;
preserve valid matching configurations and existing deprecation handling.
- Around line 1785-1807: Update the failure handling in the columnar conversion
and generic compression branches before s3_put_object(): match the live path by
returning FLB_RETRY and retaining the backlog chunk when conversion or
compression fails, rather than uploading fallback data with stale format or
Content-Encoding headers. Verify the backlog path uses the same retry behavior
as live ingestion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a018d9b3-a95c-4d33-b167-304987e43903
📒 Files selected for processing (2)
plugins/out_chronicle/chronicle.cplugins/out_s3/s3.c
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/out_chronicle/chronicle.c
1 participant
Problem
The
log_keyextraction paths inout_s3andout_chronicleallocate a scratch buffer ofbytes + bytes / 4once and never grow it. When the selectedlog_keyvalue is a non-string (number/array/object) whose JSON encoding does not fit in the remaining space,flb_msgpack_to_json()fails and the code simplybreaks out of the loop:As a result the oversized value is silently dropped (and, depending on timing, could previously be emitted truncated). This is a pre-existing data-loss bug independent of any formatting change.
Fix
Grow the scratch buffer and retry the conversion until the value fits, so an oversized
log_keyvalue is neither truncated nor dropped. The retry also guards the remaining size soflb_msgpack_to_json()is never called with a zero-length buffer.out_s3: on allocation failure it uses the function's existingalloc_errorpath.out_chronicle: on allocation failure it frees and returnsNULL.Testing
-DFLB_OUT_S3=On -DFLB_OUT_CHRONICLE=On(fullfluent-bitbinary).Context
Surfaced during review of #12129 / #12133. #12133 makes
flb_msgpack_to_json()signal truncation as a negative value (matching its contract); this PR makes these two callers actually recover from it. The two PRs are independent — this fix works whether the helper returns0or-1on overflow (it checksret <= 0).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