SidekiqClientMiddleware should continue using primary if primary was already selected
Problem
If we decide to use primary! and write wasn't performed, and we schedule a worker that would utilize load balancing capabilities, we will check against the WAL(Write Ahead Log) location of the preselected replica, instead of the primary WAL location.
When the Sidekiq server executes the worker, we will compare provided database (replica) location against the current replica, which could still be behind our primary pointer. This can lead to reading from a stale replica.
This seems to be also the reason why we see that BuildFinishedWorker
also executes some queries on the replica, although it is not supposed to, since data_consistency for this worker is set to :always
What does this MR do?
If you are currently connected to the primary and did not perform a write, you should still use primary, as this is your current read location of data.
This MR addresses this by replacing
if Session.current.performed_write?
with:
if Session.current.use_primary?
Screenshots (strongly suggested)
Does this MR meet the acceptance criteria?
Conformity
-
I have included a changelog entry, or it's not needed. (Does this MR need a changelog?) - It doesn't, because it doesn't introduce any user-facing changes. -
I have added/updated documentation, or it's not needed. (Is documentation required?) -
I have properly separated EE content from FOSS, or this MR is FOSS only. (Where should EE code go?) -
I have added information for database reviewers in the MR description, or it's not needed. (Does this MR have database related changes?) -
I have self-reviewed this MR per code review guidelines. -
This MR does not harm performance, or I have asked a reviewer to help assess the performance impact. (Merge request performance guidelines) -
I have followed the style guides.
Availability and Testing
-
I have added/updated tests following the Testing Guide, or it's not needed. (Consider all test levels. See the Test Planning Process.) -
I have tested this MR in all supported browsers, or it's not needed. -
I have informed the Infrastructure department of a default or new setting change per definition of done, or it's not needed.
Security
Does this MR contain changes to processing or storing of credentials or tokens, authorization and authentication methods or other items described in the security review guidelines? If not, then delete this Security section.
-
Label as security and @ mention @gitlab-com/gl-security/appsec
-
The MR includes necessary changes to maintain consistency between UI, API, email, or other methods -
Security reports checked/validated by a reviewer from the AppSec team
Related to #330612 (closed)