Revert "Merge branch 'fix/sm/fix_pages_worker' into 'master'"

This reverts commit 6846dad292bd7693859012a5b27158d1a995ddae.

5 files changed, 14 insertions, 94 deletions

diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb
index 00fdd047208..c760bd3b626 100644
--- a/app/services/projects/update_pages_service.rb
+++ b/app/services/projects/update_pages_service.rb
@@ -1,8 +1,5 @@
 module Projects
   class UpdatePagesService < BaseService
-    InvaildStateError = Class.new(StandardError)
-    FailedToExtractError = Class.new(StandardError)
-
     BLOCK_SIZE = 32.kilobytes
     MAX_SIZE = 1.terabyte
     SITE_PATH = 'public/'.freeze
@@ -14,15 +11,13 @@ module Projects
     end
 
     def execute
-      register_attempt
-
       # Create status notifying the deployment of pages
       @status = create_status
       @status.enqueue!
       @status.run!
 
-      raise InvaildStateError, 'missing pages artifacts' unless build.artifacts?
-      raise InvaildStateError, 'pages are outdated' unless latest?
+      raise 'missing pages artifacts' unless build.artifacts?
+      raise 'pages are outdated' unless latest?
 
       # Create temporary directory in which we will extract the artifacts
       FileUtils.mkdir_p(tmp_path)
@@ -31,22 +26,24 @@ module Projects
 
         # Check if we did extract public directory
         archive_public_path = File.join(archive_path, 'public')
-        raise FailedToExtractError, 'pages miss the public folder' unless Dir.exist?(archive_public_path)
-        raise InvaildStateError, 'pages are outdated' unless latest?
+        raise 'pages miss the public folder' unless Dir.exist?(archive_public_path)
+        raise 'pages are outdated' unless latest?
 
         deploy_page!(archive_public_path)
         success
       end
-    rescue InvaildStateError, FailedToExtractError => e
+    rescue => e
       register_failure
       error(e.message)
+    ensure
+      register_attempt
+      build.erase_artifacts! unless build.has_expiring_artifacts?
     end
 
     private
 
     def success
       @status.success
-      delete_artifact!
       super
     end
 
@@ -55,7 +52,6 @@ module Projects
       @status.allow_failure = !latest?
       @status.description = message
       @status.drop(:script_failure)
-      delete_artifact!
       super
     end
 
@@ -76,7 +72,7 @@ module Projects
       elsif artifacts.ends_with?('.zip')
         extract_zip_archive!(temp_path)
       else
-        raise FailedToExtractError, 'unsupported artifacts format'
+        raise 'unsupported artifacts format'
       end
     end
 
@@ -85,17 +81,17 @@ module Projects
                                %W(dd bs=#{BLOCK_SIZE} count=#{blocks}),
                                %W(tar -x -C #{temp_path} #{SITE_PATH}),
                                err: '/dev/null')
-      raise FailedToExtractError, 'pages failed to extract' unless results.compact.all?(&:success?)
+      raise 'pages failed to extract' unless results.compact.all?(&:success?)
     end
 
     def extract_zip_archive!(temp_path)
-      raise FailedToExtractError, 'missing artifacts metadata' unless build.artifacts_metadata?
+      raise 'missing artifacts metadata' unless build.artifacts_metadata?
 
       # Calculate page size after extract
       public_entry = build.artifacts_metadata_entry(SITE_PATH, recursive: true)
 
       if public_entry.total_size > max_size
-        raise FailedToExtractError, "artifacts for pages are too large: #{public_entry.total_size}"
+        raise "artifacts for pages are too large: #{public_entry.total_size}"
       end
 
       # Requires UnZip at least 6.00 Info-ZIP.
@@ -104,7 +100,7 @@ module Projects
       # We add * to end of SITE_PATH, because we want to extract SITE_PATH and all subdirectories
       site_path = File.join(SITE_PATH, '*')
       unless system(*%W(unzip -qq -n #{artifacts} #{site_path} -d #{temp_path}))
-        raise FailedToExtractError, 'pages failed to extract'
+        raise 'pages failed to extract'
       end
     end
 
@@ -167,11 +163,6 @@ module Projects
       build.artifacts_file.path
     end
 
-    def delete_artifact!
-      build.reload # Reload stable object to prevent erase artifacts with old state
-      build.erase_artifacts! unless build.has_expiring_artifacts?
-    end
-
     def latest_sha
       project.commit(build.ref).try(:sha).to_s
     end
diff --git a/app/workers/pages_worker.rb b/app/workers/pages_worker.rb
index 66a0ff83bef..d3b95009364 100644
--- a/app/workers/pages_worker.rb
+++ b/app/workers/pages_worker.rb
@@ -1,7 +1,7 @@
 class PagesWorker
   include ApplicationWorker
 
-  sidekiq_options retry: 3
+  sidekiq_options retry: false
 
   def perform(action, *arg)
     send(action, *arg) # rubocop:disable GitlabSecurity/PublicSend
diff --git a/changelogs/unreleased/fix-sm-fix_pages_worker.yml b/changelogs/unreleased/fix-sm-fix_pages_worker.yml
deleted file mode 100644
index 190c7d3e83e..00000000000
--- a/changelogs/unreleased/fix-sm-fix_pages_worker.yml
+++ /dev/null
@@ -1,5 +0,0 @@
----
-title: Fix pages flaky failure by reloading stale object
-merge_request: 17522
-author:
-type: fixed
diff --git a/spec/features/projects/pages_spec.rb b/spec/features/projects/pages_spec.rb
index 73fdb449557..2e334caa98f 100644
--- a/spec/features/projects/pages_spec.rb
+++ b/spec/features/projects/pages_spec.rb
@@ -57,54 +57,4 @@ feature 'Pages' do
 
     it_behaves_like 'no pages deployed'
   end
-
-  describe 'Remove page' do
-    context 'when user is the owner' do
-      let(:project) { create :project, :repository }
-
-      before do
-        project.namespace.update(owner: user)
-      end
-
-      context 'when pages are deployed' do
-        let(:pipeline) do
-          commit_sha = project.commit('HEAD').sha
-
-          project.pipelines.create(
-            ref: 'HEAD',
-            sha: commit_sha,
-            source: :push,
-            protected: false
-          )
-        end
-
-        let(:ci_build) do
-          create(
-            :ci_build,
-            project: project,
-            pipeline: pipeline,
-            ref: 'HEAD',
-            legacy_artifacts_file: fixture_file_upload(Rails.root.join('spec/fixtures/pages.zip')),
-            legacy_artifacts_metadata: fixture_file_upload(Rails.root.join('spec/fixtures/pages.zip.meta'))
-          )
-        end
-
-        before do
-          result = Projects::UpdatePagesService.new(project, ci_build).execute
-          expect(result[:status]).to eq(:success)
-          expect(project).to be_pages_deployed
-        end
-
-        it 'removes the pages' do
-          visit project_pages_path(project)
-
-          expect(page).to have_link('Remove pages')
-
-          click_link 'Remove pages'
-
-          expect(project.pages_deployed?).to be_falsey
-        end
-      end
-    end
-  end
 end
diff --git a/spec/services/projects/update_pages_service_spec.rb b/spec/services/projects/update_pages_service_spec.rb
index 934106627a9..bfb86284d86 100644
--- a/spec/services/projects/update_pages_service_spec.rb
+++ b/spec/services/projects/update_pages_service_spec.rb
@@ -34,7 +34,6 @@ describe Projects::UpdatePagesService do
           context 'with expiry date' do
             before do
               build.artifacts_expire_in = "2 days"
-              build.save!
             end
 
             it "doesn't delete artifacts" do
@@ -106,7 +105,6 @@ describe Projects::UpdatePagesService do
         context 'with expiry date' do
           before do
             build.artifacts_expire_in = "2 days"
-            build.save!
           end
 
           it "doesn't delete artifacts" do
@@ -161,20 +159,6 @@ describe Projects::UpdatePagesService do
 
         expect(execute).not_to eq(:success)
       end
-
-      context 'when timeout happens by DNS error' do
-        before do
-          allow_any_instance_of(described_class)
-            .to receive(:extract_zip_archive!).and_raise(SocketError)
-        end
-
-        it 'raises an error' do
-          expect { execute }.to raise_error(SocketError)
-
-          build.reload
-          expect(build.artifacts?).to eq(true)
-        end
-      end
     end
   end