From 006e61ea5898ad470a7ef295895ca8b83e60a1ef Mon Sep 17 00:00:00 2001
From: Jeremy Daer <jeremy@37signals.com>
Date: Mon, 12 Jan 2026 10:30:18 -0800
Subject: [PATCH] Allow ActionText embed reuse and safe purge (#2346)

---
 .../initializers/active_storage_no_reuse.rb   | 35 +++++----
 ...active_storage_purge_on_last_attachment.rb | 40 ++++++++++
 script/migrations/backfill-storage-ledger.rb  |  6 +-
 test/models/storage/no_reuse_test.rb          | 77 +++++++++++++++++++
 4 files changed, 140 insertions(+), 18 deletions(-)
 create mode 100644 config/initializers/active_storage_purge_on_last_attachment.rb

diff --git a/config/initializers/active_storage_no_reuse.rb b/config/initializers/active_storage_no_reuse.rb
index 2def087fe..0bd057cde 100644
--- a/config/initializers/active_storage_no_reuse.rb
+++ b/config/initializers/active_storage_no_reuse.rb
@@ -16,6 +16,8 @@
 # attached to an untracked type (avatar/export) could theoretically be reused in a tracked
 # context. This is acceptable - user-accessible blob IDs from untracked contexts are
 # basically nonexistent in practice.
+#
+# Exception: ActionText embeds are allowed to reuse blobs to support copy/paste.
 
 ActiveSupport.on_load(:active_storage_attachment) do
   validate :blob_account_matches_record, on: :create
@@ -29,30 +31,31 @@ ActiveSupport.on_load(:active_storage_attachment) do
     # bypass tenancy checks via try(:account) returning nil - this is intentional as
     # these classes don't participate in storage tracking.
     def blob_account_matches_record
-      return unless record&.try(:account).present?
-      return if whitelisted_for_cross_account?
-
-      unless blob&.account_id == record.account.id
-        errors.add(:blob_id, "blob account must match record account")
+      if record&.try(:account).present? && !whitelisted_for_cross_account?
+        unless blob&.account_id == record.account.id
+          errors.add(:blob_id, "blob account must match record account")
+        end
       end
     end
 
     # Ledger integrity: blob can only have one tracked attachment
     def no_tracked_blob_reuse
       tracked_record = record&.try(:storage_tracked_record)
-      return unless tracked_record.present?
-      return if whitelisted_for_cross_account?
+      if tracked_record.present? &&
+          !whitelisted_for_cross_account? &&
+          !(record_type == "ActionText::RichText" && name == "embeds")
 
-      # Check for existing attachment of this blob in tracked contexts
-      # Uses Storage::TRACKED_RECORD_TYPES constant to stay generic
-      existing = ActiveStorage::Attachment
-        .where(blob_id: blob_id)
-        .where(record_type: Storage::TRACKED_RECORD_TYPES)
-        .where.not(id: id)
-        .exists?
+        # Check for existing attachment of this blob in tracked contexts
+        # Uses Storage::TRACKED_RECORD_TYPES constant to stay generic
+        existing = ActiveStorage::Attachment
+          .where(blob_id: blob_id)
+          .where(record_type: Storage::TRACKED_RECORD_TYPES)
+          .where.not(id: id)
+          .exists?
 
-      if existing
-        errors.add(:blob_id, "cannot reuse blob in tracked storage context")
+        if existing
+          errors.add(:blob_id, "cannot reuse blob in tracked storage context")
+        end
       end
     end
 
diff --git a/config/initializers/active_storage_purge_on_last_attachment.rb b/config/initializers/active_storage_purge_on_last_attachment.rb
new file mode 100644
index 000000000..cbe0f0f13
--- /dev/null
+++ b/config/initializers/active_storage_purge_on_last_attachment.rb
@@ -0,0 +1,40 @@
+# Fizzy-specific override: ActiveStorage's default purge path uses `delete`,
+# which skips attachment callbacks. We need `destroy` so storage ledger detaches
+# are recorded and reused blobs (ActionText embeds) aren't purged until the
+# last attachment is gone. Keep this local to Fizzy; it's not a Rails default.
+module ActiveStorage
+  module PurgeOnLastAttachment
+    def purge
+      @purge_mode = :purge
+      destroy
+      purge_blob_if_last(:purge) if destroyed?
+    ensure
+      @purge_mode = nil
+    end
+
+    def purge_later
+      @purge_mode = :purge_later
+      destroy
+      purge_blob_if_last(:purge_later) if destroyed?
+    ensure
+      @purge_mode = nil
+    end
+
+    private
+      def purge_dependent_blob_later
+        if dependent == :purge_later && !@purge_mode
+          purge_blob_if_last(:purge_later)
+        end
+      end
+
+      def purge_blob_if_last(mode)
+        if blob && !blob.attachments.exists?
+          mode == :purge ? blob.purge : blob.purge_later
+        end
+      end
+  end
+end
+
+ActiveSupport.on_load(:active_storage_attachment) do
+  prepend ActiveStorage::PurgeOnLastAttachment
+end
diff --git a/script/migrations/backfill-storage-ledger.rb b/script/migrations/backfill-storage-ledger.rb
index 5d89342fc..aa3ec793f 100644
--- a/script/migrations/backfill-storage-ledger.rb
+++ b/script/migrations/backfill-storage-ledger.rb
@@ -10,17 +10,19 @@
 #
 # Safe to re-run: skips attachments that already have entries (by blob_id + recordable).
 #
-# IMPORTANT: Before running in production, verify zero historic blob reuse in tracked contexts:
+# OPTIONAL: If you want to enforce no-reuse for direct attachments, verify there are
+# no existing violations (ActionText embeds may legitimately reuse blobs):
 #
 #   ActiveStorage::Attachment
 #     .joins(:blob)
 #     .where(record_type: Storage::TRACKED_RECORD_TYPES)
+#     .where.not(record_type: "ActionText::RichText")
 #     .where.not(active_storage_blobs: { account_id: Storage::TEMPLATE_ACCOUNT_ID })
 #     .select(:blob_id)
 #     .group(:blob_id)
 #     .having("COUNT(*) > 1")
 #     .count
-#   # Should return empty hash if no reuse exists in tracked contexts
+#   # Should return empty hash if no direct-attachment reuse exists
 #
 # If reuse exists (excluding template blobs), fix the data first.
 class BackfillStorageLedger
diff --git a/test/models/storage/no_reuse_test.rb b/test/models/storage/no_reuse_test.rb
index 5af07511f..e876fe50c 100644
--- a/test/models/storage/no_reuse_test.rb
+++ b/test/models/storage/no_reuse_test.rb
@@ -33,6 +33,83 @@ class Storage::NoReuseTest < ActiveSupport::TestCase
     assert_equal 1, ActiveStorage::Attachment.where(blob_id: blob.id).count
   end
 
+  test "allows reuse for ActionText embeds" do
+    file = file_fixture("moon.jpg")
+    blob = ActiveStorage::Blob.create_and_upload! \
+      io: file.open,
+      filename: "embed.jpg",
+      content_type: "image/jpeg"
+
+    embed_html = ActionText::Attachment.from_attachable(blob).to_html
+
+    card1 = @board.cards.create!(title: "Card 1", creator: users(:david))
+    card1.update!(description: "<p>#{embed_html}</p>")
+    card1.reload
+
+    card2 = @board.cards.create!(title: "Card 2", creator: users(:david))
+    card2.update!(description: "<p>#{embed_html}</p>")
+    card2.reload
+
+    assert_equal 2, ActiveStorage::Attachment.where(
+      record_type: "ActionText::RichText",
+      name: "embeds",
+      blob_id: blob.id
+    ).count
+  end
+
+  test "purge_later does not purge blob when still attached elsewhere" do
+    file = file_fixture("moon.jpg")
+    blob = ActiveStorage::Blob.create_and_upload! \
+      io: file.open,
+      filename: "embed.jpg",
+      content_type: "image/jpeg"
+
+    embed_html = ActionText::Attachment.from_attachable(blob).to_html
+
+    card1 = @board.cards.create!(title: "Card 1", creator: users(:david))
+    card1.update!(description: "<p>#{embed_html}</p>")
+
+    card2 = @board.cards.create!(title: "Card 2", creator: users(:david))
+    card2.update!(description: "<p>#{embed_html}</p>")
+
+    attachment = ActiveStorage::Attachment.find_by(
+      record: card1.rich_text_description,
+      name: "embeds",
+      blob_id: blob.id
+    )
+
+    assert_no_enqueued_jobs only: ActiveStorage::PurgeJob do
+      attachment.purge_later
+    end
+
+    assert ActiveStorage::Blob.exists?(blob.id)
+    assert_equal 1, ActiveStorage::Attachment.where(blob_id: blob.id).count
+  end
+
+  test "purge_later enqueues purge when last attachment is removed" do
+    file = file_fixture("moon.jpg")
+    blob = ActiveStorage::Blob.create_and_upload! \
+      io: file.open,
+      filename: "embed.jpg",
+      content_type: "image/jpeg"
+
+    embed_html = ActionText::Attachment.from_attachable(blob).to_html
+
+    card = @board.cards.create!(title: "Card", creator: users(:david))
+    card.update!(description: "<p>#{embed_html}</p>")
+    card.reload
+
+    attachment = ActiveStorage::Attachment.find_by(
+      record: card.rich_text_description,
+      name: "embeds",
+      blob_id: blob.id
+    )
+
+    assert_enqueued_with job: ActiveStorage::PurgeJob, args: [ blob ] do
+      attachment.purge_later
+    end
+  end
+
   test "rejects cross-account blob attachment" do
     other_account = Account.create!(name: "Other")
     other_board = other_account.boards.create!(name: "Other Board", creator: users(:david))