Skip to content

tests: cover framed Snappy chunk aggregation and HTTP ingestion - #12565

Open
tcoatswo wants to merge 2 commits into
fluent:masterfrom
tcoatswo:fix/snappy-framed-chunk-aggregation
Open

tcoatswo wants to merge 2 commits into
fluent:masterfrom
tcoatswo:fix/snappy-framed-chunk-aggregation

Conversation

@tcoatswo

@tcoatswo tcoatswo commented Oct 12, 2026 •

Copy link
Copy Markdown

Summary

Upstream has implemented bounded Snappy decoding (flb_snappy_uncompress_bounded) via commit 8b100a5. This PR has been rebased on upstream/master to drop the superseded changes in src/flb_snappy.c while retaining test coverage for multi-chunk / uncompressed framed Snappy chunk aggregation and HTTP ingestion.

Changes

  • Unit test coverage (tests/internal/compression.c): Added test_snappy_framed_aggregation to test single uncompressed, multiple uncompressed, single compressed, mixed compressed/uncompressed, and corrupted checksum framed chunks against flb_snappy_uncompress_framed_data.
  • Integration test coverage (tests/integration/scenarios/in_http/tests/test_in_http_001.py): Added test_snappy_framed_uncompressed_data parameterized for single and multiple chunk framed Snappy ingestion over HTTP.

Fluent Bit is licensed under Apache 2.0; by submitting this pull request I understand that this code will be released under that license.

Summary by CodeRabbit

  • Tests
    • Added coverage for Snappy-framed HTTP requests with one or two uncompressed chunks, verifying successful forwarding of the expected message.
    • Added checks for decoding single and mixed compressed/uncompressed chunks, including rejection of chunks with invalid checksums.

@coderabbitai

coderabbitai Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

The changes add internal and HTTP integration tests for Snappy framed data. They cover single and multiple chunks, mixed compressed and uncompressed chunks, checksum failures, and successful HTTP forwarding.

Changes: Framed Snappy aggregation

Layer / File(s) Summary
Framed Snappy chunk test coverage
tests/internal/compression.c, tests/integration/scenarios/in_http/tests/test_in_http_001.py
Internal tests check decoded output and size for single and multiple chunks, including mixed compressed and uncompressed chunks. They also check that corrupt checksums return an error with null output and zero size. The HTTP test checks that one or two uncompressed chunks return HTTP 201 and forward the expected record.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Other

Merge Risk: Minimal · up to 464dc

This change adds test coverage for framed Snappy decoding and HTTP ingestion and does not alter runtime behavior. No merge-blocking risk was identified.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title "tests: cover framed Snappy chunk aggregation and HTTP ingestion" matches the changeset. The raw summary shows added tests in tests/internal/compression.c for framed Snappy aggregation (one …
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Full details: Title check

Explanation

The title "tests: cover framed Snappy chunk aggregation and HTTP ingestion" matches the changeset. The raw summary shows added tests in tests/internal/compression.c for framed Snappy aggregation (one and two chunks, mixed compressed/uncompressed, bad checksums) and an HTTP ingestion test for framed uncompressed Snappy data in test_in_http_001.py. The PR objectives describe the same test coverage, along with a decoder length fix that the title does not mention. The title covers a real and central part of the change but omits the underlying fix, so it is partially related rather than fully related. It is specific and not vague, so it does not fail or fall to inconclusive.

Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

Recent review info

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 50f893b9-9b4c-46d8-b820-76a4d5e8e4ee
Commits

Reviewing files that changed from the base of the PR and between 9b41d01 and 464dc93.

Files selected for processing: 2

  • tests/integration/scenarios/in_http/tests/test_in_http_001.py
  • tests/internal/compression.c

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.

@tcoatswo
tcoatswo marked this pull request as ready for review October 12, 2026 23:11
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 12, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-12T23:16:54.172047Z 9b41d01 Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

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

Copy link
Copy Markdown

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: 9b41d01d9d

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

unsigned char framed[] = {
FLB_SNAPPY_FRAME_TYPE_STREAM_IDENTIFIER, 6, 0, 0, 's', 'N', 'a', 'P', 'p', 'Y',
FLB_SNAPPY_FRAME_TYPE_UNCOMPRESSED_DATA, 7, 0, 0,
0x6e, 0x57, 0xf1, 0x21, 'a', 'b', 'c',

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Decode framed checksums as little-endian before this test

On the big-endian s390x entry in .github/workflows/unit-tests.yaml:277-280 (which runs all internal CTests at lines 324-336), the first new success case cannot pass: flb_snappy_uncompress_framed_data() loads frame_checksum through native-endian uint32_t dereferences at src/flb_snappy.c:223 and src/flb_snappy.c:270, so the wire bytes 6e 57 f1 21 are seen as 0x6e57f121 instead of 0x21f1576e. This makes flb-it-compression fail in that matrix; decode the checksum with an unaligned-safe little-endian read.

Useful? React with 👍 / 👎.

Comment thread src/flb_snappy.c Outdated
}
else {
if (aggregated_data_length > 0) {
if (result == 0 && aggregated_data_length > 0) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Reject streams that never supplied a valid Snappy identifier

When an HTTP body starts with a 0xff chunk whose length is not 6 (for example ff 00 00 00) and then carries a checksum-valid uncompressed JSON chunk, the identifier branch at src/flb_snappy.c:190-202 leaves both result == 0 and stream_identifier_found == false. This newly enabled allocation therefore copies and returns the JSON, so the HTTP input forwards a stream that never contained the required sNaPpY identifier; before this change the reset made this slow path yield zero bytes. Reject this state before either output path and cover it as an invalid envelope.

AGENTS.md reference: AGENTS.md:L120-L120

Useful? React with 👍 / 👎.

Signed-off-by: Tyler Coatsworth <14064505+tcoatswo@users.noreply.github.com>
Signed-off-by: Tyler Coatsworth <14064505+tcoatswo@users.noreply.github.com>
@tcoatswo
tcoatswo force-pushed the fix/snappy-framed-chunk-aggregation branch from 9b41d01 to 464dc93 Compare November 12, 2026 16:28
@tcoatswo tcoatswo changed the title snappy: preserve decoded length across framed chunks tests: cover framed Snappy chunk aggregation and HTTP ingestion Nov 12, 2026
@tcoatswo

Copy link
Copy Markdown
Author

Rebased onto upstream/master. Since the underlying decoder logic was consolidated upstream via flb_snappy_uncompress_bounded, the superseded changes in src/flb_snappy.c have been dropped. This PR now cleanly provides test coverage for multi-chunk / uncompressed framed Snappy chunk aggregation in tests/internal/compression.c and framed HTTP ingestion in tests/integration/scenarios/in_http/tests/test_in_http_001.py.

This branch has not been deployed

No deployments
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