out_s3: fix retry_limit semantics and multipart upload memory leaks#11669
out_s3: fix retry_limit semantics and multipart upload memory leaks#11669singholt wants to merge 3 commits intofluent:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDetects whether an output instance's Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Reviewed commit: 244de09d77
ℹ️ 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".
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/out_s3/s3.c (1)
3841-3847:⚠️ Potential issue | 🟠 MajorThe queued flush path still has the old cutoff.
This discard check was updated, but when
cb_s3_flush()sends a ready file throughs3_upload_queue(), the queue still drops onupload_contents->retry_counter >= ctx->ins->retry_limitat Line 1946. Withpreserve_data_ordering=trueby default, that path still allows onlyNtotal attempts instead ofN + 1.As per coding guidelines, "Before patching: trace one full path for affected signals (input -> chunk -> task -> output -> engine completion)".Suggested follow-up
- if (upload_contents->retry_counter >= ctx->ins->retry_limit) { + if (upload_contents->retry_counter > ctx->ins->retry_limit) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/out_s3/s3.c` around lines 3841 - 3847, The queued flush path (cb_s3_flush -> s3_upload_queue) still drops uploads when upload_contents->retry_counter >= ctx->ins->retry_limit which enforces only N attempts; change that check to match the discard logic used elsewhere (use > ctx->ins->retry_limit rather than >=) so a file gets retry_limit + 1 total attempts (honoring preserve_data_ordering behavior). Update the conditional in s3_upload_queue that checks upload_contents->retry_counter against ctx->ins->retry_limit to use the same comparison semantics as the upload_file discard 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 `@plugins/out_s3/s3.c`:
- Around line 654-658: The fallback to MAX_UPLOAD_ERRORS should only run when
the user did not set retry_limit; change the condition around
flb_output_get_property("retry_limit", ins) so you do not overwrite an
explicitly parsed ctx->ins->retry_limit (including negative
FLB_OUT_RETRY_UNLIMITED). In practice, remove the "|| ctx->ins->retry_limit < 0"
check and leave the assignment to ctx->ins->retry_limit = MAX_UPLOAD_ERRORS only
when flb_output_get_property("retry_limit", ins) == NULL so the parsed value
from ctx->ins->retry_limit is honored.
In `@tests/runtime/out_s3.c`:
- Line 496: The test uses a fixed store_dir in the flb_output_set call (out_ffd)
which causes cross-run state leakage and flaky PutObject counts; change the test
setup in tests/runtime/out_s3.c to create a unique temporary directory for
store_dir (or explicitly remove/clean the directory before and after each test)
and pass that path to flb_output_set for the S3 output instance (the same code
that sets "store_dir" for out_ffd and any other out_* instances near those
calls); ensure cleanup runs even on failures so TEST_PutObject_CALL_COUNT is
only influenced by the current test run.
---
Outside diff comments:
In `@plugins/out_s3/s3.c`:
- Around line 3841-3847: The queued flush path (cb_s3_flush -> s3_upload_queue)
still drops uploads when upload_contents->retry_counter >= ctx->ins->retry_limit
which enforces only N attempts; change that check to match the discard logic
used elsewhere (use > ctx->ins->retry_limit rather than >=) so a file gets
retry_limit + 1 total attempts (honoring preserve_data_ordering behavior).
Update the conditional in s3_upload_queue that checks
upload_contents->retry_counter against ctx->ins->retry_limit to use the same
comparison semantics as the upload_file discard path.
🪄 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: 577d876d-8a83-4f7b-9298-854d9c921c0d
📒 Files selected for processing (2)
plugins/out_s3/s3.ctests/runtime/out_s3.c
singholt
force-pushed
the
fix/s3-retry-limit-semantics
branch
from
244de09 to
ed39078
Compare
singholt
force-pushed
the
fix/s3-retry-limit-semantics
branch
from
ed39078 to
90258d7
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 (1)
plugins/out_s3/s3.c (1)
3351-3356:⚠️ Potential issue | 🟠 Major
preserve_data_orderingstill keeps the old retry boundary.These
>changes only fix the timer-based path.s3_upload_queue()still usesupload_contents->retry_counter >= ctx->ins->retry_limiton Line 1950, so queued uploads in the defaultpreserve_data_ordering=truemode still allow onlyNtotal attempts instead ofN+1. Change that check to>too so both internal retry paths agree.Suggested fix
- if (upload_contents->retry_counter >= ctx->ins->retry_limit) { + if (upload_contents->retry_counter > ctx->ins->retry_limit) { flb_plg_warn(ctx->ins, "Chunk file failed to send %d times, will not " "retry", upload_contents->retry_counter);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/out_s3/s3.c` around lines 3351 - 3356, s3_upload_queue() currently uses a >= comparison (upload_contents->retry_counter >= ctx->ins->retry_limit) which keeps the old retry boundary; change that check to use > so it matches the timer-based path (as used where chunk->failures > ctx->ins->retry_limit) — update the condition in s3_upload_queue to compare upload_contents->retry_counter > ctx->ins->retry_limit so both retry paths allow N+1 attempts and remain consistent.
♻️ Duplicate comments (1)
plugins/out_s3/s3.c (1)
654-661:⚠️ Potential issue | 🟠 MajorOnly fall back when
Retry_Limitwas omitted.
retry_limit_is_setalready distinguishes the unset case. The extractx->ins->retry_limit < 0branch rewrites an explicitRetry_Limit no_limits/off/falsetoMAX_UPLOAD_ERRORS, so the plugin no longer honors the parsed value.Suggested fix
- if (ins->retry_limit_is_set == FLB_FALSE || ctx->ins->retry_limit < 0) { + if (ins->retry_limit_is_set == FLB_FALSE) { ctx->ins->retry_limit = MAX_UPLOAD_ERRORS; }Based on learnings, "In the Fluent Bit S3 plugin, the user prefers to maintain current retry_limit behavior without special handling for FLB_OUT_RETRY_UNLIMITED (-1), as there's no documentation indicating -1 should be used for infinite retries and consistency with current logic is preferred."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/out_s3/s3.c` around lines 654 - 661, The code currently overrides an explicitly set negative retry value because the condition checks both retry_limit_is_set and ctx->ins->retry_limit < 0; change the logic to only apply the MAX_UPLOAD_ERRORS default when the user omitted Retry_Limit by removing the ctx->ins->retry_limit < 0 check so that retry_limit_is_set (and any explicit negative value such as FLB_OUT_RETRY_UNLIMITED) is honored; update the block that assigns ctx->ins->retry_limit = MAX_UPLOAD_ERRORS to run only when ins->retry_limit_is_set == FLB_FALSE, referencing retry_limit_is_set and ctx->ins->retry_limit and keeping MAX_UPLOAD_ERRORS as the fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@plugins/out_s3/s3.c`:
- Around line 3351-3356: s3_upload_queue() currently uses a >= comparison
(upload_contents->retry_counter >= ctx->ins->retry_limit) which keeps the old
retry boundary; change that check to use > so it matches the timer-based path
(as used where chunk->failures > ctx->ins->retry_limit) — update the condition
in s3_upload_queue to compare upload_contents->retry_counter >
ctx->ins->retry_limit so both retry paths allow N+1 attempts and remain
consistent.
---
Duplicate comments:
In `@plugins/out_s3/s3.c`:
- Around line 654-661: The code currently overrides an explicitly set negative
retry value because the condition checks both retry_limit_is_set and
ctx->ins->retry_limit < 0; change the logic to only apply the MAX_UPLOAD_ERRORS
default when the user omitted Retry_Limit by removing the ctx->ins->retry_limit
< 0 check so that retry_limit_is_set (and any explicit negative value such as
FLB_OUT_RETRY_UNLIMITED) is honored; update the block that assigns
ctx->ins->retry_limit = MAX_UPLOAD_ERRORS to run only when
ins->retry_limit_is_set == FLB_FALSE, referencing retry_limit_is_set and
ctx->ins->retry_limit and keeping MAX_UPLOAD_ERRORS as the fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3e72dbc-35c0-44a5-adc6-cf6040f332b6
📒 Files selected for processing (4)
include/fluent-bit/flb_output.hplugins/out_s3/s3.csrc/flb_output.ctests/runtime/out_s3.c
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/runtime/out_s3.c`:
- Line 479: The calls to mkdtemp(store_dir) are unchecked (the return value is
ignored), so if mkdtemp fails the test will proceed with an invalid store_dir;
update both occurrences (the mkdtemp calls that use the store_dir variable) to
capture the return value, check for NULL, and handle the failure by calling a
test-fatal/abort path (e.g., fprintf(stderr, ...) and exit/fail the test) or
using the existing test framework's failure macro so the test stops immediately
with a clear error message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
singholt
force-pushed
the
fix/s3-retry-limit-semantics
branch
from
90258d7 to
10c67c1
Compare
singholt
force-pushed
the
fix/s3-retry-limit-semantics
branch
from
10c67c1 to
6e95fd8
Compare
singholt
force-pushed
the
fix/s3-retry-limit-semantics
branch
from
cde7e4d to
3492dd2
Compare
singholt
changed the title
Add a boolean to flb_output_instance that tracks whether retry_limit was explicitly set by the user. This allows plugins to distinguish between the engine default and an explicit user configuration. Co-authored-by: Thean Lim <theanlim@amazon.com> Signed-off-by: Anuj Singh <singholt@amazon.com>
Fix retry_limit off-by-one: change all six >= comparisons to > so retry_limit=N correctly allows N retries (N+1 total attempts), matching engine semantics. Default retry_limit to MAX_UPLOAD_ERRORS (5) when the user has not explicitly set it, using the new retry_limit_is_set flag. The engine default of 1 is too low for S3's internal retry system where partially uploaded multipart data is wasted when retries are exhausted too early. Log a warning when capping an explicit unlimited setting to MAX_UPLOAD_ERRORS. Fix multipart upload memory leaks: - Add multipart_upload_destroy after mk_list_del when completion errors exceed retry_limit (80KB per abandoned upload) - Free s3_file before flb_fstore_file_inactive when chunk failures exceed retry_limit (48 bytes per abandoned chunk) - Free the json chunk buffer in all cb_s3_flush return paths Fix mock_s3_call if/else chain so CreateMultipartUpload returns a valid XML response with UploadId. Skip size validation in test mode so multipart paths can be exercised with small data. Remove unit_test_flush bypass so tests exercise the real flush path. Add mock call counter and skip the timer floor in test mode for faster execution. Co-authored-by: Thean Lim <theanlim@amazon.com> Signed-off-by: Anuj Singh <singholt@amazon.com>
Add retry_limit tests: - putobject_retry_limit_semantics: verify retry_limit=1 results in exactly 2 PutObject attempts - default_retry_limit: verify default retry_limit (5) results in 6 PutObject attempts Add assertions to all existing tests to verify the correct S3 API path is exercised (PutObject vs multipart) and the expected number of API calls are made. Add preserve_data_ordering test to exercise the upload queue path. Use upload_timeout=6s with sleep(10) consistently so the timer fires and exercises the real upload path. Add unique store_dir via mkdtemp to multipart tests. Clean up all API call counters at the end of each test. All 15 tests pass under valgrind with 0 bytes definitely lost. Co-authored-by: Thean Lim <theanlim@amazon.com> Signed-off-by: Anuj Singh <singholt@amazon.com>
singholt
force-pushed
the
fix/s3-retry-limit-semantics
branch
from
0a1aef5 to
9f06b6f
Compare
2 participants
Background
The S3 plugin has its own internal buffer and retry system separate from the engine. When the engine flushes a chunk, cb_s3_flush writes data to its own filesystem store and returns FLB_OK immediately. The actual S3 upload happens later via a periodic timer callback (cb_s3_upload), which tracks failures internally and checks against retry_limit. When retry attempts are exhausted, chunks are orphaned on disk — not cleanly dropped like engine-managed retries.
For multipart uploads, the cost of exhausting retries is especially high — if 8 out of 10 parts have been uploaded and the 9th fails, the entire upload is abandoned. The already-uploaded parts are wasted and sit on S3 until lifecycle rules clean them up.
Problem
The change in f4108db (#10825) replaced the hardcoded MAX_UPLOAD_ERRORS (5) with ctx->ins->retry_limit to honor user config. This introduced two issues:
The plugin used >= to compare failures against retry_limit, while the engine uses > semantics where retry_limit=N means N retries after the initial attempt. With >=, retry_limit=1 resulted in 0 retries (1 total attempt).
The engine default for retry_limit is 1, which is too low for S3's internal retry system. With default settings (60s timer ticks), chunks are abandoned after just 2 attempts ~60 seconds apart. For multipart uploads, this means
a single transient S3 error can cause an entire in-progress upload to be abandoned along with all its already-uploaded parts. The original hardcoded default of 5 gave ~5 minutes of retry window, enough to ride out transient S3 errors.
Additionally, the multipart upload error paths had memory leaks:
When completion errors exceed retry_limit, mk_list_del removes the upload from the list but multipart_upload_destroy is never called, leaking ~80KB per abandoned upload (the etags array). Over days of running, this causes OOM kills.
When chunk failures exceed retry_limit, flb_fstore_file_inactive frees the fstore file but not the s3_file context attached to it, leaking 48 bytes per abandoned chunk.
The JSON chunk buffer allocated in cb_s3_flush was not freed on several return paths.
Evidence
Stability tests running Fluent Bit v4.2.3 with S3 multipart uploads showed tasks being OOM-killed (exit code 137) after running for days. CloudWatch logs confirmed SIGSEGV in get_upload → strcmp on a dangling pointer, and core
dump analysis showed the crash at s3.c:1668 iterating ctx->uploads after an upload was removed without being destroyed.
Valgrind analysis of the S3 tests confirmed:
Changes
Testing
All tests use upload_timeout=6s with sleep(10) and unique store_dir via mkdtemp for isolation. All 15 tests pass under valgrind with 0 bytes definitely lost.
Retry limit tests:
Multipart tests now exercise the real multipart upload path and assert call counts:
PutObject, compression, and preserve_data_ordering tests assert the correct API path is used.
Documentation
Backporting