out_cloudwatch_logs: isolate stream caches per output worker - #12427
ParakhJaggi wants to merge 2 commits into
Conversation
Signed-off-by: ParakhJaggi <parakhjaggi@gmail.com>
Signed-off-by: ParakhJaggi <parakhjaggi@gmail.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughChangesCloudWatch stream cache isolation
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant CloudWatchOutput
participant OutputThreadInstance
participant StreamCache
CloudWatchOutput->>OutputThreadInstance: Resolve active worker
OutputThreadInstance-->>CloudWatchOutput: Return worker thread id
CloudWatchOutput->>StreamCache: Select worker-local cache
StreamCache-->>CloudWatchOutput: Return or refresh log stream
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The worker-local cache selection and cleanup paths are consistent with the output worker lifecycle. No current merge-blocking risk was identified. 🚥 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0069bd6448
ℹ️ 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".
cloudwatch_logsoutput workers sharectx->streamsand mutate it without synchronization. After an entry's four-hour expiration, one worker can unlink/free it while another worker traverses or unlinks it. This crashes collectors that use multiple output workers.Allocate one stream-cache list per configured output worker and select it by the existing thread-pool worker ID. Keep the instance cache for
workers 0. Destroy all caches after the output worker pool stops. Stream expiry, timestamps, and entity state then belong to one worker. This preserves parallel output processing and avoids holding a mutex across asynchronous CloudWatch requests.Compatibility: configuration and record contents stay unchanged. Each worker can cache the same CloudWatch stream independently, so cache memory and initial CreateLogStream calls can increase with worker count. The existing ResourceAlreadyExists handling covers duplicate stream creation.
Evidence
A local reproduction used synthetic dummy inputs, a HTTPS CloudWatch mock, static dummy AWS credentials,
add_entity false, andworkers 2. A clock shim advancedtime()by 14,405 seconds every five real seconds to exercise expiry repeatedly. It did not change the system clock or contact AWS.SIGSEGV,si_code=SEGV_MAPERR, address0x8.mk_list_del()received an entry with both list pointers already null:The fixed two-worker build completed a bounded 180-second accelerated-expiry run without a crash. A snapshot before the run ended showed 40,005 delivered records and zero output errors, retries, or drops. The test harness then stopped the process with SIGTERM.
Tests
Three new runtime cases cover
workers 0,workers 1, and independent ownership/expiry for two workers. The isolation test selects worker identities on an otherwise idle real output context. It deterministically fails against the original shared-cache lookup (first != second), then passes with this change. It expires one worker's entry repeatedly while the other worker's entry remains valid. Shutdown exercises cache cleanup.ctest --test-dir /build -R '^flb-rt-out_cloudwatch$' --output-on-failurePASS: all 19 CloudWatch runtime cases, 46.26 seconds.
PASS on Linux aarch64: zero errors, zero bytes allocated at exit, 18,252 allocations and frees. No suppressions were used. No CloudWatch scenario exists under
tests/integration; the focused runtime suite and the local HTTPS mock provide coverage. Full PR-range commit-prefix lint andgit diff --checkalso pass.This is separate from the entity-buffer memory leak in #12426. It does not fix the AWS credential-refresh race reported in #12206.
Summary by CodeRabbit
Bug Fixes
Tests