Merge branch '32351-fix-caching-bug' into 'master'

Expire CurrentSettings when checking job status

See merge request gitlab-org/gitlab!23202

app/controllers/admin/application_settings_controller.rb

@@ -3,7 +3,13 @@
 class Admin::ApplicationSettingsController < Admin::ApplicationController
   include InternalRedirect
 
+  # NOTE: Use @application_setting in this controller when you need to access
+  # application_settings after it has been modified. This is because the
+  # ApplicationSetting model uses Gitlab::ThreadMemoryCache for caching and the
+  # cache might be stale immediately after an update.
+  # https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/30233
   before_action :set_application_setting
+
   before_action :whitelist_query_limiting, only: [:usage_data]
   before_action :validate_self_monitoring_feature_flag_enabled, only: [
     :create_self_monitoring_project,
@@ -79,6 +85,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
     redirect_to ::Gitlab::LetsEncrypt.terms_of_service_url
   end
 
+  # Specs are in spec/requests/self_monitoring_project_spec.rb
   def create_self_monitoring_project
     job_id = SelfMonitoringProjectCreateWorker.perform_async
 
@@ -88,6 +95,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
     }
   end
 
+  # Specs are in spec/requests/self_monitoring_project_spec.rb
   def status_create_self_monitoring_project
     job_id = params[:job_id].to_s
 
@@ -98,10 +106,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
       }
     end
 
-    if Gitlab::CurrentSettings.self_monitoring_project_id.present?
-      return render status: :ok, json: self_monitoring_data
-
-    elsif SelfMonitoringProjectCreateWorker.in_progress?(job_id)
+    if SelfMonitoringProjectCreateWorker.in_progress?(job_id)
       ::Gitlab::PollingInterval.set_header(response, interval: 3_000)
 
       return render status: :accepted, json: {
@@ -109,12 +114,17 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
       }
     end
 
+    if @application_setting.self_monitoring_project_id.present?
+      return render status: :ok, json: self_monitoring_data
+    end
+
     render status: :bad_request, json: {
       message: _('Self-monitoring project does not exist. Please check logs ' \
         'for any error messages')
     }
   end
 
+  # Specs are in spec/requests/self_monitoring_project_spec.rb
   def delete_self_monitoring_project
     job_id = SelfMonitoringProjectDeleteWorker.perform_async
 
@@ -124,6 +134,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
     }
   end
 
+  # Specs are in spec/requests/self_monitoring_project_spec.rb
   def status_delete_self_monitoring_project
     job_id = params[:job_id].to_s
 
@@ -134,12 +145,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
       }
     end
 
-    if Gitlab::CurrentSettings.self_monitoring_project_id.nil?
-      return render status: :ok, json: {
-        message: _('Self-monitoring project has been successfully deleted')
-      }
-
-    elsif SelfMonitoringProjectDeleteWorker.in_progress?(job_id)
+    if SelfMonitoringProjectDeleteWorker.in_progress?(job_id)
       ::Gitlab::PollingInterval.set_header(response, interval: 3_000)
 
       return render status: :accepted, json: {
@@ -147,6 +153,12 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
       }
     end
 
+    if @application_setting.self_monitoring_project_id.nil?
+      return render status: :ok, json: {
+        message: _('Self-monitoring project has been successfully deleted')
+      }
+    end
+
     render status: :bad_request, json: {
       message: _('Self-monitoring project was not deleted. Please check logs ' \
         'for any error messages')
@@ -161,8 +173,8 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
 
   def self_monitoring_data
     {
-      project_id: Gitlab::CurrentSettings.self_monitoring_project_id,
-      project_full_path: Gitlab::CurrentSettings.self_monitoring_project&.full_path
+      project_id: @application_setting.self_monitoring_project_id,
+      project_full_path: @application_setting.self_monitoring_project&.full_path
     }
   end
 

spec/requests/self_monitoring_project_spec.rb

@@ -68,6 +68,8 @@ describe 'Self-Monitoring project requests' do
           let(:job_id) { nil }
 
           it 'returns bad_request' do
+            create(:application_setting)
+
             subject
 
             aggregate_failures do
@@ -81,11 +83,10 @@ describe 'Self-Monitoring project requests' do
         end
 
         context 'when self-monitoring project exists' do
-          let(:project) { build(:project) }
+          let(:project) { create(:project) }
 
           before do
-            stub_application_setting(self_monitoring_project_id: 1)
-            stub_application_setting(self_monitoring_project: project)
+            create(:application_setting, self_monitoring_project_id: project.id)
           end
 
           it 'does not need job_id' do
@@ -94,7 +95,7 @@ describe 'Self-Monitoring project requests' do
             aggregate_failures do
               expect(response).to have_gitlab_http_status(:success)
               expect(json_response).to eq(
-                'project_id' => 1,
+                'project_id' => project.id,
                 'project_full_path' => project.full_path
               )
             end
@@ -106,7 +107,7 @@ describe 'Self-Monitoring project requests' do
             aggregate_failures do
               expect(response).to have_gitlab_http_status(:success)
               expect(json_response).to eq(
-                'project_id' => 1,
+                'project_id' => project.id,
                 'project_full_path' => project.full_path
               )
             end
@@ -179,7 +180,7 @@ describe 'Self-Monitoring project requests' do
 
         context 'when self-monitoring project exists and job does not exist' do
           before do
-            stub_application_setting(self_monitoring_project_id: 1)
+            create(:application_setting, self_monitoring_project_id: create(:project).id)
           end
 
           it 'returns bad_request' do
@@ -196,6 +197,10 @@ describe 'Self-Monitoring project requests' do
         end
 
         context 'when self-monitoring project does not exist' do
+          before do
+            create(:application_setting)
+          end
+
           it 'does not need job_id' do
             get status_delete_self_monitoring_project_admin_application_settings_path