Add latest changes from gitlab-org/gitlab@16-6-stable-ee

9 files changed, 86 insertions, 20 deletions

diff --git a/app/models/pages_deployment.rb b/app/models/pages_deployment.rb
index 2aa36a94171..0d87a8f6cf6 100644
--- a/app/models/pages_deployment.rb
+++ b/app/models/pages_deployment.rb
@@ -5,6 +5,9 @@ class PagesDeployment < ApplicationRecord
   include EachBatch
   include FileStoreMounter
   include Gitlab::Utils::StrongMemoize
+  include SafelyChangeColumnDefault
+
+  columns_changing_default :upload_ready
 
   attribute :file_store, :integer, default: -> { ::Pages::DeploymentUploader.default_store }
 
@@ -18,7 +21,12 @@ class PagesDeployment < ApplicationRecord
   scope :with_files_stored_remotely, -> { where(file_store: ::ObjectStorage::Store::REMOTE) }
   scope :project_id_in, ->(ids) { where(project_id: ids) }
   scope :with_path_prefix, ->(prefix) { where("COALESCE(path_prefix, '') = ?", prefix.to_s) }
-  scope :active, -> { where(deleted_at: nil).order(created_at: :desc) }
+
+  # We have to mark the PagesDeployment upload as ready to ensure we only
+  # serve PagesDeployment which files are already uploaded.
+  scope :upload_ready, -> { where(upload_ready: true) }
+
+  scope :active, -> { upload_ready.where(deleted_at: nil).order(created_at: :desc) }
   scope :deactivated, -> { where('deleted_at < ?', Time.now.utc) }
 
   validates :file, presence: true
@@ -64,6 +72,18 @@ class PagesDeployment < ApplicationRecord
     return unless previous_changes.key?(:file)
 
     store_file_now!
+    mark_upload_as_finished!
+  end
+
+  # We have to mark the PagesDeployment upload as ready to ensure we only
+  # serve PagesDeployment which files are already uploaded.
+  #
+  # This is required because we're uploading the file outside of the db transaction
+  # (https://gitlab.com/gitlab-org/gitlab/-/merge_requests/114774)
+  def mark_upload_as_finished!
+    return unless file && file.exists?
+
+    update_column(:upload_ready, true)
   end
 end
 
diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb
index 83b28840d39..fd6c9a86540 100644
--- a/app/services/projects/update_pages_service.rb
+++ b/app/services/projects/update_pages_service.rb
@@ -98,7 +98,8 @@ module Projects
         file_count: deployment_update.entries_count,
         file_sha256: build.job_artifacts_archive.file_sha256,
         ci_build_id: build.id,
-        root_directory: build.options[:publish]
+        root_directory: build.options[:publish],
+        upload_ready: false
       }
     end
 
diff --git a/db/migrate/20231115091119_add_upload_finished_to_pages_deployments.rb b/db/migrate/20231115091119_add_upload_finished_to_pages_deployments.rb
new file mode 100644
index 00000000000..1165bd11874
--- /dev/null
+++ b/db/migrate/20231115091119_add_upload_finished_to_pages_deployments.rb
@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+class AddUploadFinishedToPagesDeployments < Gitlab::Database::Migration[2.2]
+  milestone '16.7'
+  enable_lock_retries!
+
+  def change
+    # Existing deployments must be considered `upload_ready` For this reason,
+    # the column is created with `default: true`, and then changed to
+    # `default: false` in post_migrate/20231115151449_update_pages_deployments_upload_ready_default_value.rb.
+    # This way, existing deployments are set as `upload_ready: true`,
+    # but new ones are created as `upload_ready: false`
+    add_column :pages_deployments, :upload_ready, :boolean, default: true
+  end
+end
diff --git a/db/post_migrate/20231115151449_update_pages_deployments_upload_ready_default_value.rb b/db/post_migrate/20231115151449_update_pages_deployments_upload_ready_default_value.rb
new file mode 100644
index 00000000000..9ab44611499
--- /dev/null
+++ b/db/post_migrate/20231115151449_update_pages_deployments_upload_ready_default_value.rb
@@ -0,0 +1,14 @@
+# frozen_string_literal: true
+
+class UpdatePagesDeploymentsUploadReadyDefaultValue < Gitlab::Database::Migration[2.2]
+  milestone '16.7'
+  enable_lock_retries!
+
+  def up
+    change_column_default :pages_deployments, :upload_ready, from: true, to: false
+  end
+
+  def down
+    change_column_default :pages_deployments, :upload_ready, from: false, to: true
+  end
+end
diff --git a/db/schema_migrations/20231115091119 b/db/schema_migrations/20231115091119
new file mode 100644
index 00000000000..9e1669953fd
--- /dev/null
+++ b/db/schema_migrations/20231115091119
@@ -0,0 +1 @@
+8974454db089c031509a6218134ac6c6bb53bd3a028f0ba83e7329d7d6445a59
\ No newline at end of file
diff --git a/db/schema_migrations/20231115151449 b/db/schema_migrations/20231115151449
new file mode 100644
index 00000000000..01d16b2a310
--- /dev/null
+++ b/db/schema_migrations/20231115151449
@@ -0,0 +1 @@
+493ba17c2dd4519c734c7ea31c2c9c6651c73b0892bd17e8fade699491f78a89
\ No newline at end of file
diff --git a/db/structure.sql b/db/structure.sql
index 1055e902056..dd784030356 100644
--- a/db/structure.sql
+++ b/db/structure.sql
@@ -20744,6 +20744,7 @@ CREATE TABLE pages_deployments (
     path_prefix text,
     build_ref text,
     deleted_at timestamp with time zone,
+    upload_ready boolean DEFAULT false,
     CONSTRAINT check_4d04b8dc9a CHECK ((char_length(path_prefix) <= 128)),
     CONSTRAINT check_5f9132a958 CHECK ((size IS NOT NULL)),
     CONSTRAINT check_7e938c810a CHECK ((char_length(root_directory) <= 255)),
diff --git a/spec/models/pages_deployment_spec.rb b/spec/models/pages_deployment_spec.rb
index 1e6f8b33a86..fdd320f13ad 100644
--- a/spec/models/pages_deployment_spec.rb
+++ b/spec/models/pages_deployment_spec.rb
@@ -71,6 +71,16 @@ RSpec.describe PagesDeployment, feature_category: :pages do
     end
   end
 
+  describe '#upload_ready' do
+    it 'marks #upload_ready as true when upload finishes' do
+      deployment = build(:pages_deployment)
+
+      expect { deployment.save! }
+        .to change { deployment.upload_ready }
+        .from(false).to(true)
+    end
+  end
+
   describe '.deactivate_all', :freeze_time do
     let!(:deployment) { create(:pages_deployment, project: project, updated_at: 5.minutes.ago) }
     let!(:nil_path_prefix_deployment) { create(:pages_deployment, project: project, path_prefix: nil) }
diff --git a/spec/uploaders/pages/deployment_uploader_spec.rb b/spec/uploaders/pages/deployment_uploader_spec.rb
index a5fe2dfe9ba..efca5bd0cf1 100644
--- a/spec/uploaders/pages/deployment_uploader_spec.rb
+++ b/spec/uploaders/pages/deployment_uploader_spec.rb
@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Pages::DeploymentUploader do
+RSpec.describe Pages::DeploymentUploader, feature_category: :pages do
   let(:pages_deployment) { create(:pages_deployment) }
   let(:uploader) { described_class.new(pages_deployment, :file) }
 
@@ -22,38 +22,41 @@ RSpec.describe Pages::DeploymentUploader do
       stub_pages_object_storage
     end
 
-    it_behaves_like 'builds correct paths', store_dir: %r[\A\h{2}/\h{2}/\h{64}/pages_deployments/\d+\z]
+    describe '.default_store' do
+      it 'returns remote store when object storage is enabled' do
+        expect(described_class.default_store).to eq(ObjectStorage::Store::REMOTE)
+      end
+    end
 
     it 'preserves original file when stores it' do
       uploader.store!(file)
 
       expect(File.exist?(file.path)).to be true
     end
+
+    it_behaves_like 'builds correct paths',
+      store_dir: %r[\A\h{2}/\h{2}/\h{64}/pages_deployments/\d+\z]
   end
 
   context 'when file is stored in valid local_path' do
-    before do
-      uploader.store!(file)
+    describe '.default_store' do
+      it 'returns local store when object storage is not enabled' do
+        expect(described_class.default_store).to eq(ObjectStorage::Store::LOCAL)
+      end
     end
 
-    subject { uploader.file.path }
-
-    it { is_expected.to match(%r[#{uploader.root}/@hashed/\h{2}/\h{2}/\h{64}/pages_deployments/#{pages_deployment.id}/pages.zip]) }
-
-    it 'preserves original file when stores it' do
-      expect(File.exist?(file.path)).to be true
-    end
-  end
+    it 'builds the right file path' do
+      uploader.store!(file)
 
-  describe '.default_store' do
-    it 'returns local store when object storage is not enabled' do
-      expect(described_class.default_store).to eq(ObjectStorage::Store::LOCAL)
+      expect(uploader.file.path).to match(
+        %r[#{uploader.root}/@hashed/\h{2}/\h{2}/\h{64}/pages_deployments/#{pages_deployment.id}/pages.zip]
+      )
     end
 
-    it 'returns remote store when object storage is enabled' do
-      stub_pages_object_storage
+    it 'preserves original file when stores it' do
+      uploader.store!(file)
 
-      expect(described_class.default_store).to eq(ObjectStorage::Store::REMOTE)
+      expect(File.exist?(file.path)).to be true
     end
   end
 end