[7.0-stable] Merge pull request #2612 from sascha-karnatz/harden-page-publisher

Gemfile

@@ -27,6 +27,11 @@ group :development, :test do
     # Necessary because GH Actions gem cache does not have this "Bundled with Ruby" gem installed
     gem "rexml", "~> 3.2.4"
     gem "sassc", "~> 2.4.0" # https://github.com/sass/sassc-ruby/issues/146
+
+    # https://github.com/hotwired/turbo-rails/issues/512
+    if rails_version == "7.1"
+      gem "actioncable", "~> #{rails_version}.0"
+    end
   else
     gem "launchy"
     gem "annotate"

app/models/alchemy/page/publisher.rb

@@ -20,21 +20,23 @@ def initialize(page)
       #
       def publish!(public_on:)
         Page.transaction do
-          version = public_version(public_on)
-          DeleteElements.new(version.elements).call
-
-          repository = page.draft_version.element_repository
-          ActiveRecord::Base.no_touching do
-            Element.acts_as_list_no_update do
-              repository.visible.not_nested.each.with_index(1) do |element, position|
-                Alchemy::DuplicateElement.new(element, repository: repository).call(
-                  page_version_id: version.id,
-                  position: position
-                )
+          PageMutex.with_lock!(@page) do
+            version = public_version(public_on)
+            DeleteElements.new(version.elements).call
+
+            repository = page.draft_version.element_repository
+            ActiveRecord::Base.no_touching do
+              Element.acts_as_list_no_update do
+                repository.visible.not_nested.each.with_index(1) do |element, position|
+                  Alchemy::DuplicateElement.new(element, repository: repository).call(
+                    page_version_id: version.id,
+                    position: position
+                  )
+                end
               end
             end
+            page.update(published_at: public_on)
           end
-          page.update(published_at: public_on)
         end
 
         Alchemy.publish_targets.each { |p| p.perform_later(page) }

app/models/alchemy/page_mutex.rb

@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+
+module Alchemy
+  class PageMutex < BaseRecord
+    class LockFailed < StandardError; end
+
+    MAX_AGE = 300 # seconds
+
+    belongs_to :page, class_name: "Alchemy::Page", optional: true
+
+    scope :expired, -> { where(arel_table[:created_at].lteq(MAX_AGE.seconds.ago)) }
+
+    def self.with_lock!(page)
+      raise ArgumentError, "A page is necessary to lock it" if page.nil?
+
+      # remove old expired page if it wasn't deleted before
+      expired.where(page: page).delete_all
+
+      begin
+        page_mutex = create!(page: page)
+      rescue ActiveRecord::RecordNotUnique
+        error = LockFailed.new("Can't lock page #{page.id} twice!")
+        logger.error error.inspect
+        raise error
+      end
+      yield
+    ensure
+      page_mutex&.destroy
+    end
+  end
+end

db/migrate/20231113104432_create_page_mutexes.rb

@@ -0,0 +1,8 @@
+class CreatePageMutexes < ActiveRecord::Migration[6.1]
+  def change
+    create_table :alchemy_page_mutexes do |t|
+      t.references :page, null: false, index: {unique: true}, foreign_key: {to_table: :alchemy_pages}
+      t.datetime :created_at
+    end
+  end
+end

spec/dummy/config/environments/test.rb

@@ -16,6 +16,11 @@
   # system, or in some way before deploying your code.
   config.eager_load = ENV["CI"].present?
 
+  # Issue: https://github.com/hotwired/turbo-rails/issues/512
+  if ENV["CI"].present?
+    Rails.autoloaders.once.do_not_eager_load("#{Turbo::Engine.root}/app/channels")
+  end
+
   # Configure public file server for tests with Cache-Control for performance.
   config.public_file_server.enabled = true
   config.public_file_server.headers = {

spec/dummy/db/migrate/20231113104432_create_page_mutexes.rb

@@ -0,0 +1,8 @@
+class CreatePageMutexes < ActiveRecord::Migration[6.1]
+  def change
+    create_table :alchemy_page_mutexes do |t|
+      t.references :page, null: false, index: {unique: true}, foreign_key: {to_table: :alchemy_pages}
+      t.datetime :created_at
+    end
+  end
+end

spec/dummy/db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.1].define(version: 2023_05_05_132743) do
+ActiveRecord::Schema[7.1].define(version: 2023_11_13_104432) do
   create_table "alchemy_attachments", force: :cascade do |t|
     t.string "name"
     t.string "file_name"
@@ -134,6 +134,12 @@
     t.index ["updater_id"], name: "index_alchemy_nodes_on_updater_id"
   end
 
+  create_table "alchemy_page_mutexes", force: :cascade do |t|
+    t.integer "page_id", null: false
+    t.datetime "created_at"
+    t.index ["page_id"], name: "index_alchemy_page_mutexes_on_page_id", unique: true
+  end
+
   create_table "alchemy_page_versions", force: :cascade do |t|
     t.integer "page_id", null: false
     t.datetime "public_on", precision: nil
@@ -283,6 +289,7 @@
   add_foreign_key "alchemy_languages", "alchemy_sites", column: "site_id"
   add_foreign_key "alchemy_nodes", "alchemy_languages", column: "language_id"
   add_foreign_key "alchemy_nodes", "alchemy_pages", column: "page_id", on_delete: :restrict
+  add_foreign_key "alchemy_page_mutexes", "alchemy_pages", column: "page_id"
   add_foreign_key "alchemy_page_versions", "alchemy_pages", column: "page_id", on_delete: :cascade
   add_foreign_key "alchemy_pages", "alchemy_languages", column: "language_id"
   add_foreign_key "alchemy_picture_thumbs", "alchemy_pictures", column: "picture_id"

spec/models/alchemy/page/publisher_spec.rb

@@ -95,5 +95,25 @@
         publish
       end
     end
+
+    context "in parallel" do
+      before do
+        # another publisher - instance created a mutex entry and locked the page
+        Alchemy::PageMutex.create(page: page, created_at: 5.seconds.ago)
+      end
+
+      it "fails, if another process locked the page" do
+        expect { publish }.to raise_error Alchemy::PageMutex::LockFailed
+      end
+
+      context "another page" do
+        let(:another_page) { create(:alchemy_page) }
+        let(:publisher) { described_class.new(another_page) }
+
+        it "should allow the publishing of another page" do
+          expect { publish }.to change { another_page.versions.published.count }.by(1)
+        end
+      end
+    end
   end
 end

spec/models/alchemy/page_mutex_spec.rb

@@ -0,0 +1,86 @@
+# frozen_string_literal: true
+
+require "rails_helper"
+
+module Alchemy
+  describe PageMutex do
+    it { is_expected.to belong_to(:page).optional }
+
+    let(:page) { create(:alchemy_page) }
+    let(:page_mutex_active) { described_class.create(page: page, created_at: 2.minutes.ago) }
+    let(:page_mutex_expired) { described_class.create(page: page, created_at: 6.minutes.ago) }
+
+    describe "#expired" do
+      it "should not have expired mutexes if one is active" do
+        page_mutex_active
+        expect(PageMutex.expired).to be_empty
+      end
+
+      it "should have expired mutexes if one is older than 5 minutes" do
+        page_mutex_expired
+        expect(PageMutex.expired.count).to eq(1)
+      end
+    end
+
+    describe "#with_lock!" do
+      context "without page" do
+        it "fires an argument error" do
+          expect { described_class.with_lock!(nil) }.to raise_error(ArgumentError)
+        end
+      end
+
+      context "without a lock" do
+        it "executes the block" do
+          expect { |block|
+            described_class.with_lock!(page, &block)
+          }.to yield_control.once
+        end
+
+        it "executes the block multiple times (sequentially)" do
+          expect { |block|
+            described_class.with_lock!(page, &block)
+            described_class.with_lock!(page, &block)
+          }.to yield_control.twice
+        end
+
+        it "return the result of the block" do
+          expect(described_class.with_lock!(page) { "foo" }).to eq("foo")
+        end
+      end
+
+      context "with a look" do
+        it "should not run in parallel and raise an exception" do
+          described_class.with_lock!(page) do
+            expect { |block|
+              described_class.with_lock!(page, &block)
+            }.to raise_error Alchemy::PageMutex::LockFailed
+          end
+        end
+
+        it "should not run if an entry is already in the database" do
+          page_mutex_active
+          expect { |block|
+            described_class.with_lock!(page, &block)
+          }.to raise_error Alchemy::PageMutex::LockFailed
+        end
+
+        it "should allow multiple mutexes for different pages" do
+          described_class.with_lock!(create(:alchemy_page)) do
+            expect { |block|
+              described_class.with_lock!(page, &block)
+            }.to yield_control.once
+          end
+        end
+      end
+
+      context "with an expired lock" do
+        it "should run if database entry is expired" do
+          page_mutex_expired
+          expect { |block|
+            described_class.with_lock!(page, &block)
+          }.to yield_control.once
+        end
+      end
+    end
+  end
+end