Skip to content

fix(child_process): fix inverted break condition in close method#5442

Open
magic-peach wants to merge 1 commit into
fluent:masterfrom
magic-peach:fix/child-process-inverted-break-condition
Open

fix(child_process): fix inverted break condition in close method#5442
magic-peach wants to merge 1 commit into
fluent:masterfrom
magic-peach:fix/child-process-inverted-break-condition

Conversation

@magic-peach

Copy link
Copy Markdown
Contributor

Summary

In the close method's wait-for-exit loop (lib/fluent/plugin_helper/child_process.rb:194), the condition break if living_process_exist is inverted and should be break unless living_process_exist.

The bug

The loop is designed to wait for all child processes to exit (or force-kill them after the kill timeout). The logic:

  1. Check all tracked pids
  2. If a process is still alive, set living_process_exist = true and force-kill it if past the timeout
  3. break if living_process_existthis is backwards

The current code breaks out of the loop as soon as it finds any living process, immediately proceeding to super and continuing shutdown without waiting for those processes to actually terminate. This allows child processes to become orphaned or zombie processes.

Comparison with shutdown method

The shutdown method (line 142) correctly implements the same wait-for-exit pattern:

if process_exists
  sleep CHILD_PROCESS_LOOP_CHECK_INTERVAL
else
  break
end

The close method should follow the same pattern: keep looping while processes are alive, break when they're all gone.

In the close method's wait-for-exit loop, the condition
'break if living_process_exist' was inverted. It should be
'break unless living_process_exist'.

The current code breaks out of the loop as soon as it finds living
processes, skipping the wait for them to actually terminate. This
allows child processes to become orphaned or zombie processes during
shutdown, leaking resources held by those processes.

The shutdown method at line 142 correctly implements this pattern:
'if process_exists: sleep; else: break'.

Signed-off-by: Akanksha Trehun <akankshatrehun@gmail.com>

@Watson1978 Watson1978 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I looked into the history before reviewing, and I believe this condition is intentional rather than an inversion. break if living_process_exist was introduced in commit 39b9060 ("fix bug to never break loop", 2016). Before that change, close was a plain while pids.size > 0 loop that could fail to terminate; the living_process_exist tracking and break if living_process_exist were added specifically to guarantee the loop always exits.

The key difference from shutdown is the loop bound. shutdown's wait loop is bounded by exit_wait_timeout:

while Fluent::Clock.now < exit_wait_timeout

so "keep looping while a process is alive" is safe there. close has no such bound. Changing it to break unless living_process_exist turns close into an unbounded "loop while any process is alive," which reintroduces exactly the non-termination bug that 39b9060 fixed: if a child can't be reaped, shutdown would hang indefinitely.

Could you share a reproduction of the orphan/zombie behavior? With that we can work out the right bounded fix. As it stands I don't think we can flip this condition, since it would re-open the 2016 hang.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants