Skip to content

multiline: ml: Propagate metadata even if on exceptions#11469

Open
cosmo0920 wants to merge 4 commits intomasterfrom
cosmo0920-propagate-metadata-in-ml
Open

multiline: ml: Propagate metadata even if on exceptions#11469
cosmo0920 wants to merge 4 commits intomasterfrom
cosmo0920-propagate-metadata-in-ml

Conversation

@cosmo0920
Copy link
Contributor

@cosmo0920 cosmo0920 commented Feb 16, 2026


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:

  • Example configuration file for the change
service:
  flush: 0.2
  log_level: info

multiline_parsers:
  - name: leading-space
    type: regex
    flush_timeout: 1000
    rules:
      - state: start_state
        regex: '/^[A-Z].*/'
        next_state: cont
      - state: cont
        regex: '/^ .*/'
        next_state: cont

pipeline:
  inputs:
    - name: opentelemetry
      listen: 0.0.0.0
      port: 14318

  filters:
    - name: multiline
      match: '*'
      multiline.parser: leading-space
      multiline.key_content: log

  outputs:
    - name: stdout
      match: '*'
  • Debug log output from testing the change
Fluent Bit v5.0.0
* Copyright (C) 2015-2025 The Fluent Bit Authors
* Fluent Bit is a CNCF graduated project under the Fluent organization
* https://fluentbit.io

______ _                  _    ______ _ _           _____  _____           _            
|  ___| |                | |   | ___ (_) |         |  ___||  _  |         | |           
| |_  | |_   _  ___ _ __ | |_  | |_/ /_| |_  __   _|___ \ | |/' |______ __| | _____   __
|  _| | | | | |/ _ \ '_ \| __| | ___ \ | __| \ \ / /   \ \|  /| |______/ _` |/ _ \ \ / /
| |   | | |_| |  __/ | | | |_  | |_/ / | |_   \ V //\__/ /\ |_/ /     | (_| |  __/\ V / 
\_|   |_|\__,_|\___|_| |_|\__| \____/|_|\__|   \_/ \____(_)\___/       \__,_|\___| \_/


[2026/02/16 20:39:05.698338000] [ info] Configuration:
[2026/02/16 20:39:05.698350000] [ info]  flush time     | 0.200000 seconds
[2026/02/16 20:39:05.698360000] [ info]  grace          | 5 seconds
[2026/02/16 20:39:05.698365000] [ info]  daemon         | 0
[2026/02/16 20:39:05.698370000] [ info] ___________
[2026/02/16 20:39:05.698376000] [ info]  inputs:
[2026/02/16 20:39:05.698380000] [ info]      opentelemetry
[2026/02/16 20:39:05.698384000] [ info] ___________
[2026/02/16 20:39:05.698394000] [ info]  filters:
[2026/02/16 20:39:05.698398000] [ info]      multiline.0
[2026/02/16 20:39:05.698402000] [ info] ___________
[2026/02/16 20:39:05.698406000] [ info]  outputs:
[2026/02/16 20:39:05.698410000] [ info]      stdout.0
[2026/02/16 20:39:05.698414000] [ info] ___________
[2026/02/16 20:39:05.698419000] [ info]  collectors:
[2026/02/16 20:39:05.698918000] [ info] [fluent bit] version=5.0.0, commit=d142e3fbd0, pid=38364
[2026/02/16 20:39:05.698926000] [debug] [engine] coroutine stack size: 36864 bytes (36.0K)
[2026/02/16 20:39:05.699109000] [ info] [storage] ver=1.5.4, type=memory, sync=normal, checksum=off, max_chunks_up=128
[2026/02/16 20:39:05.699115000] [ info] [simd    ] NEON
[2026/02/16 20:39:05.699118000] [ info] [cmetrics] version=1.0.7
[2026/02/16 20:39:05.699273000] [ info] [ctraces ] version=0.7.0
[2026/02/16 20:39:05.699507000] [ info] [input:opentelemetry:opentelemetry.0] initializing
[2026/02/16 20:39:05.699511000] [ info] [input:opentelemetry:opentelemetry.0] storage_strategy='memory' (memory only)
[2026/02/16 20:39:05.699517000] [debug] [opentelemetry:opentelemetry.0] created event channels: read=25 write=26
[2026/02/16 20:39:05.699787000] [debug] [downstream] listening on 0.0.0.0:14318
[2026/02/16 20:39:05.699792000] [ info] [input:opentelemetry:opentelemetry.0] listening on 0.0.0.0:14318
[2026/02/16 20:39:05.699968000] [ info] [filter:multiline:multiline.0] created emitter: emitter_for_multiline.0
[2026/02/16 20:39:05.699995000] [ info] [input:emitter:emitter_for_multiline.0] initializing
[2026/02/16 20:39:05.699998000] [ info] [input:emitter:emitter_for_multiline.0] storage_strategy='memory' (memory only)
[2026/02/16 20:39:05.700007000] [debug] [emitter:emitter_for_multiline.0] created event channels: read=28 write=29
[2026/02/16 20:39:05.700324000] [debug] [stdout:stdout.0] created event channels: read=31 write=32
[2026/02/16 20:39:05.700541000] [debug] [router] match rule opentelemetry.0:stdout.0
[2026/02/16 20:39:05.700545000] [debug] [router] match rule emitter.1:stdout.0
[2026/02/16 20:39:05.700552000] [ info] [output:stdout:stdout.0] worker #0 started
[2026/02/16 20:39:05.700735000] [ info] [sp] stream processor started
[2026/02/16 20:39:05.700914000] [ info] [engine] Shutdown Grace Period=5, Shutdown Input Grace Period=2
[2026/02/16 20:39:08.638537000] [ info] [filter:multiline:multiline.0] created new multiline stream for opentelemetry.0_v1_logs
[2026/02/16 20:39:08.638569000] [debug] [filter:multiline:multiline.0] Created new ML stream for opentelemetry.0_v1_logs
[2026/02/16 20:39:08.704368000] [debug] [task] created task=0x60000387c370 id=0 OK
[2026/02/16 20:39:08.704383000] [debug] [output:stdout:stdout.0] task_id=0 assigned to thread #0
[0] v1_logs: [[1700000000.18446744072918273792, {"otlp"=>{"trace_id"=>"0af7651916cd43dd8448eb211c80319c", "span_id"=>"b7ad6b7169203331"}}], {"log"=>"Error: something failed"}]
[1] v1_logs: [[1700000001.208722175, {"otlp"=>{"trace_id"=>"aaaabbbbccccdddd1111222233334444", "span_id"=>"1234567890abcdef"}}], {"log"=>"123 non-matching record"}]
[2026/02/16 20:39:08.704450000] [debug] [out flush] cb_destroy coro_id=0
[2026/02/16 20:39:08.704470000] [debug] [task] destroy task=0x60000387c370 (task_id=0)
^C[2026/02/16 20:39:10] [engine] caught signal (SIGINT)
[2026/02/16 20:39:10.38185000] [ info] [input] pausing emitter_for_multiline.0
[2026/02/16 20:39:10.38355000] [ info] [output:stdout:stdout.0] thread worker #0 stopping...
[2026/02/16 20:39:10.38512000] [ info] [output:stdout:stdout.0] thread worker #0 stopped
  • Attached Valgrind output that shows no leaks or memory corruption was found

macOS's leaks command result is:

Process 38539 is not debuggable. Due to security restrictions, leaks can only show or save contents of readonly memory of restricted processes.

Process:         fluent-bit [38539]
Path:            /Users/USER/*/fluent-bit
Load Address:    0x100238000
Identifier:      fluent-bit
Version:         0
Code Type:       ARM64
Platform:        macOS
Parent Process:  leaks [38538]
Target Type:     live task

Date/Time:       2026-02-16 20:39:53.685 +0900
Launch Time:     2026-02-16 20:39:41.925 +0900
OS Version:      macOS 15.7.3 (24G419)
Report Version:  7
Analysis Tool:   /usr/bin/leaks

Physical footprint:         6626K
Physical footprint (peak):  6706K
Idle exit:                  untracked
----

leaks Report Version: 4.0, multi-line stacks
Process 38539: 1126 nodes malloced for 165 KB
Process 38539: 0 leaks for 0 total leaked bytes.

It reported no leaks.

If this is a change to packaging of containers or native binaries then please confirm it works for all targets.

  • Run local packaging test showing all targets (including any new ones) build.
  • Set ok-package-test label to test for all targets (requires maintainer to do).

Documentation

  • Documentation required for this feature

Backporting

  • Backport to latest stable release.

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

  • Performance

    • Faster multiline log processing via improved in-memory metadata handling that reduces pack/unpack overhead and increases throughput.
  • Reliability

    • Better metadata retention, normalization and deduplication across multiline grouping, flush, error handling and teardown, ensuring consistent per-line metadata propagation and proper cleanup.

Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
@coderabbitai
Copy link

coderabbitai bot commented Feb 16, 2026

📝 Walkthrough

Walkthrough

Adds per-stream-group in-memory metadata snapshot storage and management for multiline processing: two new fields on flb_ml_stream_group and new APIs to add and purge deep-copied msgpack metadata, plus calls to preserve and clear metadata across multiline lifecycle and on destruction. (≤50 words)

Changes

Cohort / File(s) Summary
Multiline Header
include/fluent-bit/multiline/flb_ml.h
Added struct mk_list metadata_objects; and int metadata_objects_initialized;, and declarations for flb_ml_stream_group_add_metadata(...) and flb_ml_stream_group_purge_metadata(...).
Multiline Core
src/multiline/flb_ml.c
Implemented per-group metadata manager: deep-copy conversion for msgpack objects (strings, BIN→hex, ext, arrays, maps), bin_to_hex helper, APIs to add/purge metadata, deduplication/packing integration, and purge on flush/error paths.
Stream Lifecycle
src/multiline/flb_ml_stream.c
Added call to flb_ml_stream_group_purge_metadata(group) in stream_group_destroy to clear stored metadata before freeing the group.

Sequence Diagram

sequenceDiagram
    participant Producer as Content Producer
    participant MetaMgr as Metadata Manager
    participant StreamGrp as Stream Group
    participant Flusher as Flush Handler

    Producer->>MetaMgr: flb_ml_stream_group_add_metadata(metadata)
    MetaMgr->>MetaMgr: deep_copy_msgpack_object(metadata)
    MetaMgr->>StreamGrp: append metadata_objects entry

    Note over StreamGrp: metadata snapshots retained per-group until flush or destroy

    Flusher->>StreamGrp: request metadata for emit
    StreamGrp->>MetaMgr: get/deduplicate/pack metadata
    MetaMgr->>Flusher: provide packed/deduped metadata
    Flusher->>MetaMgr: flb_ml_stream_group_purge_metadata(group)
    MetaMgr->>StreamGrp: clear list & reset initialized flag
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

  • fujimotos

Poem

🐇 I hop through bytes and copy each map,

I turn odd bins to hex with a tap.
Snapshots nest tidy in lists so light,
Kept until flush, then gone from sight.
🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'multiline: ml: Propagate metadata even if on exceptions' accurately reflects the main change—extending metadata propagation to handle exception cases in the multiline filter.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch cosmo0920-propagate-metadata-in-ml

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/multiline/flb_ml.c (1)

1610-1623: ⚠️ Potential issue | 🟡 Minor

Metadata not purged on early error returns.

When the first-line state buffer unpack fails (lines 1610-1614 or 1618-1623), the function returns -1 without reaching the flb_ml_stream_group_purge_metadata(group) at line 1767. The accumulated metadata entries remain in the group's list and will be incorrectly included in the next successful flush.

🔧 Proposed fix
         if (ret != MSGPACK_UNPACK_SUCCESS) {
             flb_error("[multiline] could not unpack first line state buffer");
             msgpack_unpacked_destroy(&result);
             group->mp_sbuf.size = 0;
+            flb_ml_stream_group_purge_metadata(group);
             return -1;
         }
         map = result.data;
 
         if (map.type != MSGPACK_OBJECT_MAP) {
             flb_error("[multiline] expected MAP type in first line state buffer");
             msgpack_unpacked_destroy(&result);
             group->mp_sbuf.size = 0;
+            flb_ml_stream_group_purge_metadata(group);
             return -1;
         }
🤖 Fix all issues with AI agents
In `@src/multiline/flb_ml.c`:
- Around line 1473-1478: The deep-copy failure path leaks partially-allocated
sub-objects in entry->obj; after flb_ml_msgpack_object_deep_copy_convert(...)
returns non-zero, call the routine that frees/destroys the msgpack object (e.g.,
flb_ml_msgpack_object_destroy(&entry->obj) or the project’s equivalent
destructor) before flb_free(entry) so all heap allocations from the failed copy
are released, then return -1; keep mk_list_entry_init(&entry->_head) as-is.
- Around line 1329-1339: The MSGPACK_OBJECT_STR path currently calls
flb_malloc(len) and treats NULL as failure, which breaks when src->via.str.size
== 0 because flb_malloc(0) can return NULL; change the logic in the
MSGPACK_OBJECT_STR handling so that when src->via.str.size == 0 you do not call
flb_malloc and instead set dst->via.str.ptr to an immutable empty buffer (or
literal "") and dst->via.str.size = 0, otherwise allocate and copy as before
(use the same pattern for MSGPACK_OBJECT_BIN and MSGPACK_OBJECT_EXT referring to
dst->via.bin.ptr/dst->via.ext.ptr and src->via.bin.size/src->via.ext.size).
- Around line 1374-1419: The array and map deep-copy loops leak memory on
mid-loop failure; after allocating dst->via.array.ptr or dst->via.map.ptr and
before returning -1 when flb_ml_msgpack_object_deep_copy_convert fails, call
flb_ml_msgpack_object_destroy(dst) to free any partially-copied children and the
allocated ptr, then return -1. Apply the same fix in both the
MSGPACK_OBJECT_ARRAY loop (checking failure inside the for loop that copies into
dst->via.array.ptr[i]) and in the MSGPACK_OBJECT_MAP loop (for failures copying
dst->via.map.ptr[i].key or .val), ensuring you clean up dst via
flb_ml_msgpack_object_destroy before returning.
🧹 Nitpick comments (4)
src/multiline/flb_ml.c (4)

1563-1564: Double purge of metadata on success path.

flb_ml_flush_metadata_buffer purges metadata at line 1564, and then flb_ml_flush_stream_group purges again at line 1767. The second call is a harmless no-op but makes ownership semantics unclear. Consider removing the purge from flb_ml_flush_metadata_buffer and letting the caller own cleanup, or vice versa.

Also applies to: 1767-1767


1421-1424: Shallow copy in the default case could cause double-free for pointer-bearing types.

*dst = *src copies raw union bytes. If an unhandled type contains a pointer (unlikely today, but possible with future msgpack extensions), both src and dst would reference the same memory, leading to a double-free when flb_ml_msgpack_object_destroy is called. Consider setting dst->type = MSGPACK_OBJECT_NIL instead or returning an error.


349-351: Return value of flb_ml_stream_group_add_metadata is silently discarded.

If the deep-copy allocation fails (OOM), the metadata for this line is lost without any log message. Consider at least logging a warning on failure so operators can diagnose metadata loss.


1501-1505: Use mk_list_is_empty() instead of manual empty-list check.

Replace (group->metadata_objects.next == &group->metadata_objects) with (mk_list_is_empty(&group->metadata_objects) == 0) for consistency with the rest of the codebase.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f9b0a94ed5

ℹ️ 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".

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
src/multiline/flb_ml.c (1)

1632-1644: ⚠️ Potential issue | 🟠 Major

Memory leak: early returns bypass metadata purge.

These two early-return paths (msgpack_unpack_next failure at line 1636 and non-MAP check at line 1644) were harmless before this PR because metadata was stored in mp_md_sbuf (just a size reset). Now that metadata is a linked list of heap-allocated deep-copied msgpack_object snapshots, returning here leaks every entry in group->metadata_objects.

Additionally, the local mp_sbuf initialized at line 1615 is not destroyed on these paths (pre-existing).

🔧 Proposed fix — purge metadata and destroy local sbuf on early returns
         if (ret != MSGPACK_UNPACK_SUCCESS) {
             flb_error("[multiline] could not unpack first line state buffer");
             msgpack_unpacked_destroy(&result);
+            msgpack_sbuffer_destroy(&mp_sbuf);
+            flb_ml_stream_group_purge_metadata(group);
             group->mp_sbuf.size = 0;
             return -1;
         }
         map = result.data;

         if (map.type != MSGPACK_OBJECT_MAP) {
             flb_error("[multiline] expected MAP type in first line state buffer");
             msgpack_unpacked_destroy(&result);
+            msgpack_sbuffer_destroy(&mp_sbuf);
+            flb_ml_stream_group_purge_metadata(group);
             group->mp_sbuf.size = 0;
             return -1;
         }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/multiline/flb_ml.c` around lines 1632 - 1644, On the two early-return
paths that follow msgpack_unpack_next and the map-type check, ensure you free
the per-group metadata list and destroy the local sbuf before returning: iterate
and free every heap-allocated entry in group->metadata_objects (purge the linked
list and reset any count/head pointers) and call
msgpack_sbuffer_destroy(&mp_sbuf) for the locally-initialized mp_sbuf, then
proceed to msgpack_unpacked_destroy(&result) and set group->mp_sbuf.size = 0
before returning -1; update both failure branches (the branch after
msgpack_unpack_next failure and the non-MAP branch) to perform these cleanup
steps so no metadata or sbuf memory is leaked.
🧹 Nitpick comments (2)
src/multiline/flb_ml.c (2)

1523-1527: Consider using mk_list_is_empty for readability.

The raw pointer comparison group->metadata_objects.next == &group->metadata_objects is a correct empty-list check, but using a list API macro (if available) would be clearer.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/multiline/flb_ml.c` around lines 1523 - 1527, Replace the raw empty-list
pointer comparison in the early-return check with the list API macro for
clarity: change the condition that tests group->metadata_objects.next ==
&group->metadata_objects to use mk_list_is_empty(&group->metadata_objects)
(keeping the surrounding checks for group == NULL and
group->metadata_objects_initialized). Update the condition in the function that
contains group, metadata_objects_initialized and metadata_objects to use
mk_list_is_empty so the intent is clearer and consistent with other list usage.

1785-1790: Double metadata purge is safe but redundant.

flb_ml_flush_metadata_buffer (line 1586) unconditionally purges the metadata list. The purge at line 1789 runs on an already-empty list. This is harmless and defensive — just noting it for awareness. No action needed.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/multiline/flb_ml.c` around lines 1785 - 1790, The code calls
flb_ml_stream_group_purge_metadata twice: once unconditionally in
flb_ml_flush_metadata_buffer and again later where group->mp_md_sbuf.size is
reset; remove the redundant second call to flb_ml_stream_group_purge_metadata
(the one after resetting group->truncated and before setting
group->mp_md_sbuf.size = 0) to avoid unnecessary double-purge while leaving the
original purge in flb_ml_flush_metadata_buffer intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/multiline/flb_ml.c`:
- Around line 1759-1766: The error path after the event encoder failure in
flb_ml.c fails to destroy the local msgpack sbuffer; ensure that when ret !=
FLB_EVENT_ENCODER_SUCCESS you call msgpack_sbuffer_destroy(&mp_sbuf) before
purging metadata and returning -1. Update the error branch that currently logs
flb_error(...) and calls flb_ml_stream_group_purge_metadata(group) /
group->mp_md_sbuf.size = 0 so it first destroys mp_sbuf (via
msgpack_sbuffer_destroy) to avoid leaking resources, then proceed with the
existing cleanup and return.
- Around line 1451-1505: stream_group_destroy currently frees the group without
cleaning up metadata entries added by flb_ml_stream_group_add_metadata, leaking
flb_ml_metadata_object_entry and their msgpack objects; update
stream_group_destroy to call flb_ml_stream_group_purge_metadata(group) before
flb_free(group) so all metadata entries are removed and their resources freed
(ensure you reference the flb_ml_stream_group_purge_metadata function and call
it at the start of stream_group_destroy's teardown path).

---

Outside diff comments:
In `@src/multiline/flb_ml.c`:
- Around line 1632-1644: On the two early-return paths that follow
msgpack_unpack_next and the map-type check, ensure you free the per-group
metadata list and destroy the local sbuf before returning: iterate and free
every heap-allocated entry in group->metadata_objects (purge the linked list and
reset any count/head pointers) and call msgpack_sbuffer_destroy(&mp_sbuf) for
the locally-initialized mp_sbuf, then proceed to
msgpack_unpacked_destroy(&result) and set group->mp_sbuf.size = 0 before
returning -1; update both failure branches (the branch after msgpack_unpack_next
failure and the non-MAP branch) to perform these cleanup steps so no metadata or
sbuf memory is leaked.

---

Nitpick comments:
In `@src/multiline/flb_ml.c`:
- Around line 1523-1527: Replace the raw empty-list pointer comparison in the
early-return check with the list API macro for clarity: change the condition
that tests group->metadata_objects.next == &group->metadata_objects to use
mk_list_is_empty(&group->metadata_objects) (keeping the surrounding checks for
group == NULL and group->metadata_objects_initialized). Update the condition in
the function that contains group, metadata_objects_initialized and
metadata_objects to use mk_list_is_empty so the intent is clearer and consistent
with other list usage.
- Around line 1785-1790: The code calls flb_ml_stream_group_purge_metadata
twice: once unconditionally in flb_ml_flush_metadata_buffer and again later
where group->mp_md_sbuf.size is reset; remove the redundant second call to
flb_ml_stream_group_purge_metadata (the one after resetting group->truncated and
before setting group->mp_md_sbuf.size = 0) to avoid unnecessary double-purge
while leaving the original purge in flb_ml_flush_metadata_buffer intact.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
src/multiline/flb_ml.c (3)

1451-1457: Lazy initialization pattern is correct but could be set eagerly in stream_group_create.

The lazy-init via metadata_objects_initialized flag works because every access path (add, purge, flush) checks the flag. However, initializing the list in stream_group_create (in flb_ml_stream.c) would eliminate the flag entirely and simplify all call sites.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/multiline/flb_ml.c` around lines 1451 - 1457, The metadata list lazy-init
adds unnecessary complexity; instead eagerly initialize the list in
stream_group_create and remove the metadata_objects_initialized flag and the
helper flb_ml_stream_group_metadata_list_init. In practice: in the constructor
flb_ml_stream_group_create (in flb_ml_stream.c) call
mk_list_init(&group->metadata_objects) and stop setting or checking
metadata_objects_initialized; then remove the metadata_objects_initialized field
from struct flb_ml_stream_group and delete calls to
flb_ml_stream_group_metadata_list_init and its definition, and simplify callers
such as flb_ml_stream_group_add_metadata_object,
flb_ml_stream_group_purge_metadata and flb_ml_stream_group_flush_metadata to
assume metadata_objects is always initialized.

1585-1586: Double purge: metadata is purged both inside flb_ml_flush_metadata_buffer (line 1586) and again at end-of-flush (line 1790).

The second purge at line 1790 iterates an already-empty list and is harmless, but it's redundant on the success path. If you'd like to keep both for defensive safety that's fine — just noting it.

Also applies to: 1790-1790

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/multiline/flb_ml.c` around lines 1585 - 1586, The metadata purge is being
called twice: once inside flb_ml_flush_metadata_buffer and again via
flb_ml_stream_group_purge_metadata at the end-of-flush; remove the redundant
second call (the end-of-flush invocation) or guard it so it only runs when
metadata remains (e.g., check the group's metadata list emptiness before calling
flb_ml_stream_group_purge_metadata). Update the flush routine that currently
calls flb_ml_stream_group_purge_metadata at the end so it either deletes that
call or wraps it in a conditional, leaving the single purge inside
flb_ml_flush_metadata_buffer as the canonical cleanup.

1442-1445: Default case performs a shallow copy — safe for known msgpack scalar types only.

The *dst = *src shallow copy is fine for the remaining scalar types in the msgpack spec (which have no heap pointers). However, if a future msgpack-c version adds a new pointer-bearing type, flb_ml_msgpack_object_destroy would not free it (its default is a no-op), so the data would just be an opaque copy rather than a leak. Consider adding a log warning here for unknown types to ease future debugging.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/multiline/flb_ml.c` around lines 1442 - 1445, The switch default in the
msgpack copy (where "*dst = *src" is used) silently performs a shallow copy for
unknown/future types; change it to emit a warning so unexpected pointer-bearing
types are easier to detect: in the default branch (around the copy logic in the
function handling msgpack object copy), call the project's logging API (e.g.,
flb_warn or equivalent) to log the msgpack type tag or a brief message
identifying the unknown type and the code path, then still perform the shallow
copy to preserve current behavior; this makes issues visible to developers and
ties into flb_ml_msgpack_object_destroy investigations if a leak appears.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/multiline/flb_ml_stream.c`:
- Around line 150-151: The call to flb_ml_stream_group_purge_metadata from
flb_ml_stream.c fails because the function is defined as static in flb_ml.c and
not visible across translation units; either remove the static from the flb_ml.c
definition so it has external linkage and add a prototype to the public
multiline header (declare extern flb_ml_stream_group_purge_metadata(...) in
include header), or if the function must remain internal, move its
implementation into flb_ml_stream.c and call the local helper instead; update
the declaration/usage to keep a single non-static definition and ensure the
header contains the prototype for callers like flb_ml_stream.c.

---

Nitpick comments:
In `@src/multiline/flb_ml.c`:
- Around line 1451-1457: The metadata list lazy-init adds unnecessary
complexity; instead eagerly initialize the list in stream_group_create and
remove the metadata_objects_initialized flag and the helper
flb_ml_stream_group_metadata_list_init. In practice: in the constructor
flb_ml_stream_group_create (in flb_ml_stream.c) call
mk_list_init(&group->metadata_objects) and stop setting or checking
metadata_objects_initialized; then remove the metadata_objects_initialized field
from struct flb_ml_stream_group and delete calls to
flb_ml_stream_group_metadata_list_init and its definition, and simplify callers
such as flb_ml_stream_group_add_metadata_object,
flb_ml_stream_group_purge_metadata and flb_ml_stream_group_flush_metadata to
assume metadata_objects is always initialized.
- Around line 1585-1586: The metadata purge is being called twice: once inside
flb_ml_flush_metadata_buffer and again via flb_ml_stream_group_purge_metadata at
the end-of-flush; remove the redundant second call (the end-of-flush invocation)
or guard it so it only runs when metadata remains (e.g., check the group's
metadata list emptiness before calling flb_ml_stream_group_purge_metadata).
Update the flush routine that currently calls flb_ml_stream_group_purge_metadata
at the end so it either deletes that call or wraps it in a conditional, leaving
the single purge inside flb_ml_flush_metadata_buffer as the canonical cleanup.
- Around line 1442-1445: The switch default in the msgpack copy (where "*dst =
*src" is used) silently performs a shallow copy for unknown/future types; change
it to emit a warning so unexpected pointer-bearing types are easier to detect:
in the default branch (around the copy logic in the function handling msgpack
object copy), call the project's logging API (e.g., flb_warn or equivalent) to
log the msgpack type tag or a brief message identifying the unknown type and the
code path, then still perform the shallow copy to preserve current behavior;
this makes issues visible to developers and ties into
flb_ml_msgpack_object_destroy investigations if a leak appears.

Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (2)
src/multiline/flb_ml.c (2)

1786-1789: Redundant purge after flb_ml_flush_metadata_buffer already purges.

flb_ml_flush_metadata_buffer (line 1580) unconditionally calls flb_ml_stream_group_purge_metadata(group) before returning. The purge here on line 1788 is therefore always operating on an already-empty list. It's harmless (the iteration is a no-op on an empty mk_list), but worth noting it's defensive duplication.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/multiline/flb_ml.c` around lines 1786 - 1789, The call to
flb_ml_stream_group_purge_metadata(group) in the block that sets
flb_sds_len_set(group->buf, 0), group->truncated = FLB_FALSE and
group->mp_md_sbuf.size = 0 is redundant because flb_ml_flush_metadata_buffer
already unconditionally purges metadata; remove this duplicate
flb_ml_stream_group_purge_metadata(group) invocation (leave the buffer resets
and mp_md_sbuf.size assignment intact) to avoid unnecessary no-op iteration and
keep a single canonical purge point in flb_ml_flush_metadata_buffer.

1436-1440: Shallow-copy fallback for unknown msgpack types could silently alias pointers.

The default branch does *dst = *src, which is a shallow (bitwise) copy. While all known msgpack object types with heap pointers (STR, BIN, EXT, ARRAY, MAP) are handled above, if a future msgpack-c version adds a new pointer-bearing type, this would create a double-free on destroy. Consider logging a warning or returning an error for truly unexpected types to make debugging easier.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/multiline/flb_ml.c` around lines 1436 - 1440, The default switch branch
currently does a shallow bitwise copy (`*dst = *src`) which can alias heap
pointers; instead, in the default branch for unknown msgpack types (where `dst`
and `src` are used) refuse to shallow-copy: log a warning including the
unexpected `src->type`, zero/initialize `*dst` to a safe empty state (e.g. set
`dst->type = MSGPACK_OBJECT_NIL` or memset to 0) and return/propagate an error
code so callers know cloning failed; update the function containing that switch
(the block with `default:` handling `dst`/`src`) to avoid double-free risk and
ensure callers handle the new error path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/multiline/flb_ml.c`:
- Around line 343-345: The bug is that flb_ml_stream_group_add_metadata is
called after flb_ml_flush_stream_group may have purged metadata, leaving an
orphaned snapshot for the next record; to fix, ensure metadata is added before
any flush or avoid re-adding after a flush: move the call to
flb_ml_stream_group_add_metadata in package_content so it runs prior to any
calls to flb_ml_flush_stream_group (including those inside flb_ml_rule_process),
or alternately add a guard (e.g., track a local "flushed" flag or check
stream_group->mp_md_sbuf.size) and only call flb_ml_stream_group_add_metadata
when the group has not just been flushed; update package_content accordingly so
metadata cannot be appended after a purge.

---

Nitpick comments:
In `@src/multiline/flb_ml.c`:
- Around line 1786-1789: The call to flb_ml_stream_group_purge_metadata(group)
in the block that sets flb_sds_len_set(group->buf, 0), group->truncated =
FLB_FALSE and group->mp_md_sbuf.size = 0 is redundant because
flb_ml_flush_metadata_buffer already unconditionally purges metadata; remove
this duplicate flb_ml_stream_group_purge_metadata(group) invocation (leave the
buffer resets and mp_md_sbuf.size assignment intact) to avoid unnecessary no-op
iteration and keep a single canonical purge point in
flb_ml_flush_metadata_buffer.
- Around line 1436-1440: The default switch branch currently does a shallow
bitwise copy (`*dst = *src`) which can alias heap pointers; instead, in the
default branch for unknown msgpack types (where `dst` and `src` are used) refuse
to shallow-copy: log a warning including the unexpected `src->type`,
zero/initialize `*dst` to a safe empty state (e.g. set `dst->type =
MSGPACK_OBJECT_NIL` or memset to 0) and return/propagate an error code so
callers know cloning failed; update the function containing that switch (the
block with `default:` handling `dst`/`src`) to avoid double-free risk and ensure
callers handle the new error path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

Comments