Skip to content

engine_dispatch: Address stale failed tasks on dispatch - #12254

Open
cosmo0920 wants to merge 4 commits into
masterfrom
cosmo0920-plug-stale-failed-tasks-on-dispatch
Open

engine_dispatch: Address stale failed tasks on dispatch#12254
cosmo0920 wants to merge 4 commits into
masterfrom
cosmo0920-plug-stale-failed-tasks-on-dispatch

Conversation

@cosmo0920

@cosmo0920 cosmo0920 commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Implemented the stale-task cleanup.

Changes:

  • src/flb_engine_dispatch.c:72 now calls flb_task_users_release(task) after destroying the failed retry.
  • Added tests/internal/engine_dispatch.c covering:
    • Releasing the last task owner and reusing its task-map slot.
    • Preserving a task with an active user.
    • Preserving a task with another pending retry.
  • Registered the test in tests/internal/CMakeLists.txt.

Verification:

cmake -S . -B build -DFLB_TESTS_RUNTIME=On -DFLB_TESTS_INTERNAL=On
cmake --build build --target flb-it-engine_dispatch -j8
ctest --test-dir build -R "^(flb-it-engine_dispatch|flb-it-task_map)$" --output-on-failure

Result: both tests passed, 0 failures.

The regression test was also built and run temporarily without the production fix. It failed on the occupied task-map slot, retained task/chunk lists, and non-reused task ID. After restoring the fix, it passed.

No Python integration scenario covers this engine-dispatch lifecycle path.

Closes #12252.


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
  • Debug log output from testing the change
  • Attached Valgrind output that shows no leaks or memory corruption was found

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

  • Bug Fixes

    • Improved cleanup when retry processing fails, preventing retained task references.
    • Preserved correct task ownership and retry state during failed chunk flushing.
    • Added accurate metrics for failed retries and dropped records and bytes.
  • Tests

    • Added coverage for retry dispatch failures, including final-owner cleanup, active task preservation, pending retry handling, and metric reporting.

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

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The retry dispatch failure path now records retry and drop metrics before it destroys the failed retry and releases the task reference. Internal tests cover task ownership, retry preservation, chunk registration, metrics, and task ID reuse.

Changes

Retry dispatch failure

Layer / File(s) Summary
Record metrics and release failed retries
src/flb_engine_dispatch.c
Failed retry chunk retrieval records output, router, legacy, and drop metrics. The path then destroys the retry context and releases the task user reference.
Build isolated retry failure state
tests/internal/engine_dispatch.c, tests/internal/CMakeLists.txt
Test helpers create engine state, metrics, tasks, routes, retries, and chunks. The CMake configuration registers the test and adds the required ChunkIO include directory.
Validate ownership and registration cleanup
tests/internal/engine_dispatch.c
Tests cover final-owner release, active-owner preservation, pending-retry preservation, metric updates, task and chunk cleanup, and task ID reuse.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: edsiper

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: cleaning up stale failed tasks during engine dispatch.
Linked Issues check ✅ Passed The changes release orphaned tasks, preserve active tasks and retries, record failure metrics, and add tests for issue #12252.
Out of Scope Changes check ✅ Passed The implementation, metrics updates, test coverage, and build registration directly support the linked issue objectives.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch cosmo0920-plug-stale-failed-tasks-on-dispatch

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.

@coderabbitai coderabbitai 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.

Actionable comments posted: 1

🧹 Nitpick comments (1)
tests/internal/engine_dispatch.c (1)

30-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use the required control-statement brace style.

Move the opening brace to the next line for each new if, else, while, and do block in this file. Line 30 is one example.

Proposed fix
-    if (ctx == NULL) {
+    if (ctx == NULL)
+    {
         return;
     }
🤖 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 `@tests/internal/engine_dispatch.c` around lines 30 - 32, Update the control
statements in engine_dispatch.c to place opening braces on the following line
for every if, else, while, and do block, including the block guarding ctx ==
NULL. Preserve all existing logic and formatting otherwise.

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 `@tests/internal/engine_dispatch.c`:
- Around line 28-48: The test fixture must balance Winsock lifecycle management.
Update test_ctx_create to check the WSAStartup result and record successful
initialization in test_ctx; update test_ctx_destroy to call WSACleanup only when
initialization succeeded, after resource teardown, while preserving cleanup for
failed or partially initialized fixtures.

---

Nitpick comments:
In `@tests/internal/engine_dispatch.c`:
- Around line 30-32: Update the control statements in engine_dispatch.c to place
opening braces on the following line for every if, else, while, and do block,
including the block guarding ctx == NULL. Preserve all existing logic and
formatting otherwise.
🪄 Autofix

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 Plus

Run ID: e0471cc7-4f77-4584-8ece-706754020b33

📥 Commits

Reviewing files that changed from the base of the PR and between a1d6fb1 and e01062f.

📒 Files selected for processing (3)
  • src/flb_engine_dispatch.c
  • tests/internal/CMakeLists.txt
  • tests/internal/engine_dispatch.c

Comment thread tests/internal/engine_dispatch.c

@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: e01062fbe6

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

Comment thread src/flb_engine_dispatch.c
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>

@coderabbitai coderabbitai 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.

🧹 Nitpick comments (1)
tests/internal/engine_dispatch.c (1)

89-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use the required control-block brace style.

Move each opening brace to the next line.

  • tests/internal/engine_dispatch.c#L89-L94: Move the if (ret != 0) opening brace.
  • tests/internal/engine_dispatch.c#L139-L161: Move the counter-allocation failure-block opening brace.
  • tests/internal/engine_dispatch.c#L166-L178: Move the metrics-allocation failure-block opening brace.
  • tests/internal/engine_dispatch.c#L187-L190: Move the metrics cleanup-block opening brace.
  • tests/internal/engine_dispatch.c#L236-L244: Move the metric null-check opening braces.
  • tests/internal/engine_dispatch.c#L283-L292: Move the task-event-chunk and route-allocation failure-block opening braces.
  • tests/internal/engine_dispatch.c#L333-L338: Move the output initialization failure-block opening brace.
  • tests/internal/engine_dispatch.c#L385-L390: Move the output initialization failure-block opening brace.
  • tests/internal/engine_dispatch.c#L451-L456: Move the output initialization failure-block opening brace.
  • tests/internal/engine_dispatch.c#L470-L481: Move the allocation failure-block opening brace.

As per coding guidelines: “Put function opening braces on the next line.”

🤖 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 `@tests/internal/engine_dispatch.c` around lines 89 - 94, Apply the required
next-line brace style in tests/internal/engine_dispatch.c at lines 89-94,
139-161, 166-178, 187-190, 236-244, 283-292, 333-338, 385-390, 451-456, and
470-481: move each listed if/else failure, cleanup, null-check, and
allocation-block opening brace onto its own following line, without changing
control flow or behavior.

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.

Nitpick comments:
In `@tests/internal/engine_dispatch.c`:
- Around line 89-94: Apply the required next-line brace style in
tests/internal/engine_dispatch.c at lines 89-94, 139-161, 166-178, 187-190,
236-244, 283-292, 333-338, 385-390, 451-456, and 470-481: move each listed
if/else failure, cleanup, null-check, and allocation-block opening brace onto
its own following line, without changing control flow or behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 09d52167-1e44-4865-94be-373d3a9506a7

📥 Commits

Reviewing files that changed from the base of the PR and between e01062f and 3b29e2a.

📒 Files selected for processing (2)
  • src/flb_engine_dispatch.c
  • tests/internal/engine_dispatch.c
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/flb_engine_dispatch.c

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.

engine_dispatch: task is never released when flb_input_chunk_flush() fails, pinning its task-map slot and input chunk until restart

1 participant