Skip to content

out_opensearch: lock the client cache against flush threads - #195

Merged
Watson1978 merged 2 commits into
mainfrom
fix-flush
Sep 18, 2026
Merged

Watson1978 merged 2 commits into
mainfrom
fix-flush

Conversation

@kenhys

@kenhys kenhys commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Before: client() cleared the shared @_os slot and then rebuilt it with ||=, without a lock. With flush_thread_count of 2 or more and a host placeholder, a thread could pass the "is this the same host" check and then pick up the client another thread had just built for a different host, so its chunk went to that cluster instead.

After: the whole check-and-build runs under a mutex, so a thread always gets a client for the host it asked for. The mutex is created in initialize because configure builds a client before it returns.

Before: client() cleared the shared @_os slot and then rebuilt it with
`||=`, without a lock. With flush_thread_count of 2 or more and a host
placeholder, a thread could pass the "is this the same host" check and
then pick up the client another thread had just built for a different
host, so its chunk went to that cluster instead.

After: the whole check-and-build runs under a mutex, so a thread always
gets a client for the host it asked for. The mutex is created in
initialize because configure builds a client before it returns.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Kentaro Hayashi <hayashi@clear-code.com>
@cosmo0920

Copy link
Copy Markdown
Contributor

@kenhys This type of fix also ought to apply for https://github.com/uken/fluent-plugin-elasticsearch?
Once the fix is applied there, we need to release a new version of ES plugin as well.

@kenhys

kenhys commented Sep 17, 2026

Copy link
Copy Markdown
Contributor Author

@kenhys This type of fix also ought to apply for https://github.com/uken/fluent-plugin-elasticsearch? Once the fix is applied there, we need to release a new version of ES plugin as well.

I've overlooked it, maybe similar fix is needed.

@kenhys
kenhys marked this pull request as ready for review September 17, 2026 01:35
@kenhys
kenhys requested a review from Watson1978 September 17, 2026 01:36
@Watson1978

Copy link
Copy Markdown
Contributor

timer_execute(:out_opensearch_expire_credentials, @endpoint.refresh_credentials_interval) do
log.debug('Recreate the AWS credentials')
@credential_mutex.synchronize do
@_os = nil
begin
@_aws_credentials = aws_credentials(@endpoint)
rescue => e
log.error("Failed to get new AWS credentials: #{e}")
end
end

This code runs periodically using a timer.
It assigns a value to @_os here as well, but because a different mutex is used, a timing conflict could cause issues.
Therefore, it needs to be protected by the same mutex.

diff --git a/lib/fluent/plugin/out_opensearch.rb b/lib/fluent/plugin/out_opensearch.rb
index cd3ff78..90e6221 100644
--- a/lib/fluent/plugin/out_opensearch.rb
+++ b/lib/fluent/plugin/out_opensearch.rb
@@ -349,7 +349,6 @@ class << self
         end
       end
       # Credential providers refresh themselves near expiration. This timer recreates them forcibly.
-      @credential_mutex = Mutex.new
       if @endpoint
         @_aws_credentials = aws_credentials(@endpoint)
 
@@ -357,12 +356,14 @@ class << self
           timer_execute(:out_opensearch_expire_credentials, @endpoint.refresh_credentials_interval) do
             log.debug('Recreate the AWS credentials')
 
-            @credential_mutex.synchronize do
-              @_os = nil
-              begin
-                @_aws_credentials = aws_credentials(@endpoint)
-              rescue => e
-                log.error("Failed to get new AWS credentials: #{e}")
+            begin
+              credentials = aws_credentials(@endpoint)
+            rescue => e
+              log.error("Failed to get new AWS credentials: #{e}")
+            else
+              @client_mutex.synchronize do
+                @_aws_credentials = credentials
+                @_os = nil
               end
             end
           end

@Watson1978

Watson1978 commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

@_os = nil if @reconnect_on_error

I think the above assignment also requires the mutex.

Before:
the credential refresh timer used its own mutex and the flush
thread error path used none, so both could clear @_os while another
thread was building a client under @client_mutex. A failed credential
refresh also threw away the cached client for nothing.

After:
both paths take @client_mutex, and the timer keeps the old
credentials when it cannot get new ones.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Kentaro Hayashi <hayashi@clear-code.com>
@kenhys

kenhys commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Now follow-up review feedback

@Watson1978
Watson1978 merged commit c9387e8 into main Sep 18, 2026
23 checks passed
@Watson1978
Watson1978 deleted the fix-flush branch September 18, 2026 05:50
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.

3 participants