From bd6c1cf34ff25648c78e90e1d244768d0ddad050 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 19 Dec 2025 13:07:32 -0800 Subject: [PATCH] Storage: harden reconcile for concurrent writes and fix board transfer (#2096) * Storage: harden reconcile for concurrent writes and fix board transfer Reconcile now uses two-cursor approach: captures cursor before and after the storage scan, aborting if they differ (entries added during scan). Job retries 3x with 1-minute waits and limits concurrency to 1 per owner. Board transfer now correctly moves storage for card description embeds and comment embeds, not just direct attachments. Uses batched queries to handle cards with thousands of comments efficiently. Also fixes N+1 queries in attachment grouping via lookup maps. * Storage: fix per-attachment reconcile and enforce no blob reuse The storage ledger tracks per-attachment (not per-blob) as a business abstraction for quotas. This fixes reconcile to match that model and adds enforcement to prevent blob reuse in tracked contexts. * Reconcile now uses joins(:blob).sum() for per-attachment counting * New validation prevents reusing blobs across tracked attachments * Race-safe storage_total creation with create_or_find_by * Job efficiency: skip find_by when object already available * Backfill script checks per-attachment, not just per-blob --- app/jobs/storage/reconcile_job.rb | 6 +- app/models/board/storage.rb | 32 +++- app/models/card.rb | 49 +++++ app/models/concerns/storage/totaled.rb | 48 +++-- app/models/concerns/storage/tracked.rb | 64 +++++-- app/models/storage.rb | 13 ++ app/models/storage/attachment_tracking.rb | 19 +- app/models/storage/entry.rb | 28 ++- .../initializers/active_storage_no_reuse.rb | 64 +++++++ script/migrations/backfill-storage-ledger.rb | 21 ++- test/jobs/storage/reconcile_job_test.rb | 25 +++ test/models/storage/entry_test.rb | 49 ++--- test/models/storage/no_reuse_test.rb | 88 +++++++++ test/models/storage/totaled_test.rb | 81 +++++++++ test/models/storage/tracked_test.rb | 167 ++++++++++++++++++ 15 files changed, 669 insertions(+), 85 deletions(-) create mode 100644 config/initializers/active_storage_no_reuse.rb create mode 100644 test/models/storage/no_reuse_test.rb diff --git a/app/jobs/storage/reconcile_job.rb b/app/jobs/storage/reconcile_job.rb index be0c0748c..cd3bf3803 100644 --- a/app/jobs/storage/reconcile_job.rb +++ b/app/jobs/storage/reconcile_job.rb @@ -1,9 +1,13 @@ class Storage::ReconcileJob < ApplicationJob + class ReconcileAborted < StandardError; end + queue_as :backend + limits_concurrency to: 1, key: ->(owner) { owner } discard_on ActiveJob::DeserializationError + retry_on ReconcileAborted, wait: 1.minute, attempts: 3 def perform(owner) - owner.reconcile_storage + raise ReconcileAborted, "Could not get stable snapshot for #{owner.class}##{owner.id}" unless owner.reconcile_storage end end diff --git a/app/models/board/storage.rb b/app/models/board/storage.rb index 64dde9985..24028e714 100644 --- a/app/models/board/storage.rb +++ b/app/models/board/storage.rb @@ -12,25 +12,32 @@ module Board::Storage # Calculate actual storage by summing blob sizes. # - # Uses batched pluck queries to avoid loading huge ID arrays, and avoids - # ActiveRecord model queries on ActiveStorage tables to sidestep cross-pool - # issues when ActiveStorage uses separate connection pools (e.g., with replicas). + # Storage tracking is a business abstraction - we count what users upload. + # Original upload bytes only; variants/previews/derivatives excluded. + # Physical storage optimizations (deduplication, compression) don't affect quotas. def calculate_real_storage_bytes + @card_ids = nil # Clear memoization for fresh calculation card_image_bytes + card_embed_bytes + comment_embed_bytes + board_embed_bytes end + def card_ids + @card_ids ||= cards.pluck(:id) + end + def card_image_bytes sum_blob_bytes_in_batches \ ActiveStorage::Attachment.where(record_type: "Card", name: "image"), - cards.pluck(:id) + card_ids end def card_embed_bytes - sum_embed_bytes_for "Card", cards.pluck(:id) + sum_embed_bytes_for "Card", card_ids end def comment_embed_bytes - sum_embed_bytes_for "Comment", Comment.where(card_id: cards.pluck(:id)).pluck(:id) + card_ids.each_slice(BATCH_SIZE).sum do |batch| + sum_embed_bytes_for "Comment", Comment.where(card_id: batch).pluck(:id) + end end def board_embed_bytes @@ -48,9 +55,18 @@ module Board::Storage end def sum_blob_bytes_in_batches(base_scope, record_ids) + # Count per-attachment to match ledger model. + # Same blob attached 3 times = 3x bytes (business abstraction, not physical storage). + # + # Do NOT remove the join thinking it's a performance optimization - it's required + # for correct per-attachment counting. We keep ActiveStorage/ActionText in the same + # database (realm/geo partitioning, not functionality partitioning), so cross-table + # joins are fine. record_ids.each_slice(BATCH_SIZE).sum do |batch_ids| - blob_ids = base_scope.where(record_id: batch_ids).pluck(:blob_id) - ActiveStorage::Blob.where(id: blob_ids).sum(:byte_size) + base_scope + .where(record_id: batch_ids) + .joins(:blob) + .sum("active_storage_blobs.byte_size") end end end diff --git a/app/models/card.rb b/app/models/card.rb index 33c96e502..dbb45f184 100644 --- a/app/models/card.rb +++ b/app/models/card.rb @@ -68,6 +68,55 @@ class Card < ApplicationRecord end private + STORAGE_BATCH_SIZE = 1000 + + # Override to include comments, but only load comments that have attachments. + # Cards can have thousands of comments; most won't have attachments. + # Streams in batches to avoid loading all IDs into memory at once. + def storage_transfer_records + comment_ids_with_attachments = storage_comment_ids_with_attachments + + if comment_ids_with_attachments.any? + [ self, *comments.where(id: comment_ids_with_attachments).to_a ] + else + [ self ] + end + end + + def storage_comment_ids_with_attachments + direct = [] + rich_text_map = {} + + # Stream comment IDs in batches to avoid loading all into memory + comments.in_batches(of: STORAGE_BATCH_SIZE) do |batch| + batch_ids = batch.pluck(:id) + + direct.concat \ + ActiveStorage::Attachment + .where(record_type: "Comment", record_id: batch_ids) + .distinct + .pluck(:record_id) + + ActionText::RichText + .where(record_type: "Comment", record_id: batch_ids) + .pluck(:id, :record_id) + .each { |rt_id, comment_id| rich_text_map[rt_id] = comment_id } + end + + embed_comment_ids = if rich_text_map.any? + rich_text_map.keys.each_slice(STORAGE_BATCH_SIZE).flat_map do |batch_ids| + ActiveStorage::Attachment + .where(record_type: "ActionText::RichText", record_id: batch_ids) + .distinct + .pluck(:record_id) + end.filter_map { |rt_id| rich_text_map[rt_id] } + else + [] + end + + (direct + embed_comment_ids).uniq + end + def set_default_title self.title = "Untitled" if title.blank? end diff --git a/app/models/concerns/storage/totaled.rb b/app/models/concerns/storage/totaled.rb index 3f220f55f..b54a6ef22 100644 --- a/app/models/concerns/storage/totaled.rb +++ b/app/models/concerns/storage/totaled.rb @@ -19,7 +19,7 @@ module Storage::Totaled # Exact: snapshot + pending entries def bytes_used_exact - (storage_total || create_storage_total!).current_usage + create_or_find_storage_total.current_usage end def materialize_storage_later @@ -28,7 +28,7 @@ module Storage::Totaled # Materialize all pending entries into snapshot def materialize_storage - total = storage_total || create_storage_total! + total = create_or_find_storage_total total.with_lock do latest_entry_id = storage_entries.maximum(:id) @@ -44,26 +44,44 @@ module Storage::Totaled end # Reconcile ledger against actual attachment storage. - # Uses cursor to ensure consistency: captures max entry ID first, then calculates - # real bytes, then sums only entries up to that cursor. Concurrent uploads during - # calculation will have entries with IDs beyond the cursor, avoiding double-count. + # + # Uses two-cursor approach for consistency: capture cursor before AND after the + # scan. If they differ, entries were added during the scan and we can't get an + # accurate diff without risking double-counting or undercounting. + # + # Returns true if reconciled successfully, false if aborted due to concurrent + # writes. Caller (ReconcileJob) handles retries to avoid amplification. def reconcile_storage - max_entry_id = storage_entries.maximum(:id) + cursor_before = storage_entries.maximum(:id) real_bytes = calculate_real_storage_bytes - ledger_bytes = max_entry_id ? storage_entries.where(id: ..max_entry_id).sum(:delta) : 0 - diff = real_bytes - ledger_bytes + cursor_after = storage_entries.maximum(:id) - if diff.nonzero? - Storage::Entry.record \ - account: is_a?(Account) ? self : account, - board: is_a?(Board) ? self : nil, - recordable: nil, - delta: diff, - operation: "reconcile" + if cursor_before != cursor_after + Rails.logger.warn "[Storage] Reconcile aborted for #{self.class}##{id}: cursor moved during scan" + false + else + ledger_bytes = cursor_after ? storage_entries.where(id: ..cursor_after).sum(:delta) : 0 + diff = real_bytes - ledger_bytes + + if diff.nonzero? + Rails.logger.info "[Storage] Reconcile #{self.class}##{id}: adjusting by #{diff} bytes" + Storage::Entry.record \ + account: is_a?(Account) ? self : account, + board: is_a?(Board) ? self : nil, + recordable: nil, + delta: diff, + operation: "reconcile" + end + + true end end private + def create_or_find_storage_total + self.storage_total ||= Storage::Total.create_or_find_by!(owner: self) + end + def calculate_real_storage_bytes raise NotImplementedError, "Subclass must implement calculate_real_storage_bytes" end diff --git a/app/models/concerns/storage/tracked.rb b/app/models/concerns/storage/tracked.rb index 7b96e95c0..745ef6692 100644 --- a/app/models/concerns/storage/tracked.rb +++ b/app/models/concerns/storage/tracked.rb @@ -1,3 +1,6 @@ +# Storage tracking is a business abstraction - we count what users upload. +# Original upload bytes only; variants/previews/derivatives excluded. +# Physical storage optimizations (deduplication, compression) don't affect quotas. module Storage::Tracked extend ActiveSupport::Concern @@ -22,21 +25,27 @@ module Storage::Tracked private def board_transfer? - respond_to?(:board_id_changed?) && board_id_changed? + respond_to?(:will_save_change_to_board_id?) && will_save_change_to_board_id? end def track_board_transfer - old_board_id = board_id_was - current_bytes = storage_bytes + old_board = Board.find_by(id: attribute_in_database(:board_id)) + records = storage_transfer_records.compact + return if records.empty? + + attachments_by_record = storage_attachments_for_records(records) + + attachments_by_record.each do |recordable, attachments| + bytes = attachments.sum { |attachment| attachment.blob.byte_size } + next if bytes.zero? - if current_bytes.positive? # Debit old board - if old_board_id + if old_board Storage::Entry.record \ account: account, - board_id: old_board_id, - recordable: self, - delta: -current_bytes, + board: old_board, + recordable: recordable, + delta: -bytes, operation: "transfer_out" end @@ -44,14 +53,45 @@ module Storage::Tracked Storage::Entry.record \ account: account, board: board, - recordable: self, - delta: current_bytes, + recordable: recordable, + delta: bytes, operation: "transfer_in" end end + def storage_transfer_records + [ self ] + end + # Override if needed. Default = all direct attachments - def attachments_for_storage - ActiveStorage::Attachment.where(record: self) + def attachments_for_storage(recordable = self) + storage_attachments_for_records([ recordable ]).fetch(recordable, []) + end + + def storage_attachments_for_records(recordables) + records = Array(recordables).compact + return {} if records.empty? + + # Build lookup for records by (type, id) to avoid N+1 when resolving RichText parents + records_by_key = records.index_by { |r| [ r.class.name, r.id ] } + + rich_texts = ActionText::RichText.where(record: records) + rich_text_to_parent = rich_texts.to_h { |rt| [ rt.id, records_by_key[[ rt.record_type, rt.record_id ]] ] } + + attachments = ActiveStorage::Attachment + .where(record: records + rich_texts) + .includes(:blob) + .to_a + + attachments.each_with_object(Hash.new { |h, k| h[k] = [] }) do |attachment, grouped| + # Resolve parent without N+1: use lookup for RichText, direct for others + recordable = if attachment.record_type == "ActionText::RichText" + rich_text_to_parent[attachment.record_id] + else + records_by_key[[ attachment.record_type, attachment.record_id ]] + end + + grouped[recordable] << attachment if recordable + end end end diff --git a/app/models/storage.rb b/app/models/storage.rb index 9c17b7cd9..445362b34 100644 --- a/app/models/storage.rb +++ b/app/models/storage.rb @@ -2,4 +2,17 @@ module Storage def self.table_name_prefix "storage_" end + + # Record types that participate in storage tracking (ledger entries created on attach). + # The no-reuse validation uses this to scope its check. + # + # IMPORTANT: Update this constant when adding tracked attachments to new models. + # If you add a direct attachment (not via RichText embeds) to Comment, Board, or + # another model with Storage::Tracked, you must add its record_type here or the + # no-reuse validation won't protect it. + TRACKED_RECORD_TYPES = %w[Card ActionText::RichText].freeze + + # Account ID for template/demo blobs that can be reused cross-tenant. + # Set to nil to disable the whitelist (no exemptions). + TEMPLATE_ACCOUNT_ID = nil end diff --git a/app/models/storage/attachment_tracking.rb b/app/models/storage/attachment_tracking.rb index d68f5845d..1c3c7151d 100644 --- a/app/models/storage/attachment_tracking.rb +++ b/app/models/storage/attachment_tracking.rb @@ -26,24 +26,23 @@ module Storage::AttachmentTracking return unless @storage_snapshot Storage::Entry.record \ - account_id: @storage_snapshot[:account_id], - board_id: @storage_snapshot[:board_id], - recordable_type: @storage_snapshot[:recordable_type], - recordable_id: @storage_snapshot[:recordable_id], - blob_id: @storage_snapshot[:blob_id], + account: @storage_snapshot[:account], + board: @storage_snapshot[:board], + recordable: @storage_snapshot[:recordable], + blob: blob, delta: -blob.byte_size, operation: "detach" end + # Snapshot records in before_destroy since parent may be deleted by the time + # after_destroy_commit runs. The records may be destroyed but .id still works. def snapshot_storage_context return unless storage_tracked_record @storage_snapshot = { - account_id: storage_tracked_record.account.id, - board_id: storage_tracked_record.board_for_storage_tracking&.id, - recordable_type: storage_tracked_record.class.name, - recordable_id: storage_tracked_record.id, - blob_id: blob.id + account: storage_tracked_record.account, + board: storage_tracked_record.board_for_storage_tracking, + recordable: storage_tracked_record } end diff --git a/app/models/storage/entry.rb b/app/models/storage/entry.rb index a2f6e726b..768a4c6f8 100644 --- a/app/models/storage/entry.rb +++ b/app/models/storage/entry.rb @@ -5,30 +5,26 @@ class Storage::Entry < ApplicationRecord scope :pending, ->(last_entry_id) { where.not(id: ..last_entry_id) if last_entry_id } - # Accepts either objects or _id params (for after_destroy_commit snapshots) - def self.record(delta:, operation:, account: nil, account_id: nil, board: nil, board_id: nil, - recordable: nil, recordable_type: nil, recordable_id: nil, blob: nil, blob_id: nil) + # Records may be destroyed (during cascading deletes) but .id still works. + # Skip entirely if account is destroyed - no need to track storage for deleted accounts. + # Skip materialize jobs for destroyed boards since there's nothing to update. + def self.record(delta:, operation:, account:, board: nil, recordable: nil, blob: nil) return if delta.zero? - - account_id ||= account&.id - board_id ||= board&.id - blob_id ||= blob&.id + return if account.destroyed? entry = create! \ - account_id: account_id, - board_id: board_id, - recordable_type: recordable_type || recordable&.class&.name, - recordable_id: recordable_id || recordable&.id, - blob_id: blob_id, + account_id: account.id, + board_id: board&.id, + recordable_type: recordable&.class&.name, + recordable_id: recordable&.id, + blob_id: blob&.id, delta: delta, operation: operation, user_id: Current.user&.id, request_id: Current.request_id - # Enqueue materialization - use find_by to handle cascading deletes - # (Account/Board may be destroyed while attachments are still being cleaned up) - Account.find_by(id: account_id)&.materialize_storage_later - Board.find_by(id: board_id)&.materialize_storage_later if board_id + account.materialize_storage_later + board&.materialize_storage_later unless board&.destroyed? entry end diff --git a/config/initializers/active_storage_no_reuse.rb b/config/initializers/active_storage_no_reuse.rb new file mode 100644 index 000000000..2def087fe --- /dev/null +++ b/config/initializers/active_storage_no_reuse.rb @@ -0,0 +1,64 @@ +# Enforce storage ledger integrity by preventing blob reuse in tracked contexts. +# +# Two invariants: +# 1. Account match: blob.account_id == record.account_id (multi-tenant safety) +# 2. No reuse within tracked contexts: a blob can only have one tracked attachment +# +# With per-attachment reconcile, blob reuse inside an account wouldn't break correctness - +# ledger would still count each attach, and reconcile would agree. However, we intentionally +# forbid reuse (except templates) as a product/control decision: +# - Simpler mental model (one blob = one attachment) +# - Prevents accidental quota manipulation via direct blob_id reuse +# - Cleaner audit trail in ledger entries +# +# Scope note: The no-reuse validation only blocks reuse when the *new* attachment is tracked +# AND only checks *existing* attachments in Storage::TRACKED_RECORD_TYPES. A blob first +# 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. + +ActiveSupport.on_load(:active_storage_attachment) do + validate :blob_account_matches_record, on: :create + validate :no_tracked_blob_reuse, on: :create + + private + # Multi-tenant safety: blob must belong to same account as record + # NOTE: Skips validation if record.account is nil. This is a theoretical bypass + # if someone attaches before account assignment, but our flows assign account + # before attachment. Global/unaccounted attachments (Identity/User avatars, exports) + # 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") + 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? + + # 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") + end + end + + def whitelisted_for_cross_account? + # Only template account blobs can be reused cross-tenant. + # When TEMPLATE_ACCOUNT_ID is nil, no exemptions are granted. + Storage::TEMPLATE_ACCOUNT_ID.present? && blob&.account_id == Storage::TEMPLATE_ACCOUNT_ID + end +end diff --git a/script/migrations/backfill-storage-ledger.rb b/script/migrations/backfill-storage-ledger.rb index 9ad0b20b4..5d89342fc 100644 --- a/script/migrations/backfill-storage-ledger.rb +++ b/script/migrations/backfill-storage-ledger.rb @@ -8,7 +8,21 @@ # Run via Kamal: # kamal app exec -d -p --reuse "bin/rails runner script/migrations/backfill-storage-ledger.rb" # -# Safe to re-run: skips attachments that already have entries (by blob_id). +# 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: +# +# ActiveStorage::Attachment +# .joins(:blob) +# .where(record_type: Storage::TRACKED_RECORD_TYPES) +# .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 +# +# If reuse exists (excluding template blobs), fix the data first. class BackfillStorageLedger def run puts "Backfilling storage entries…" @@ -26,7 +40,10 @@ class BackfillStorageLedger ActiveStorage::Attachment.includes(:blob).find_each do |attachment| record = attachment.record.try(:storage_tracked_record) - if record.nil? || Storage::Entry.exists?(blob_id: attachment.blob_id) + # Backfill creates one entry PER ATTACHMENT (not per blob) to match the ledger model. + # Storage tracking is a business abstraction at the attachment level. + # IMPORTANT: This assumes no historic blob reuse. Run pre-check query above first. + if record.nil? || Storage::Entry.exists?(blob_id: attachment.blob_id, recordable: record) skipped += 1 next end diff --git a/test/jobs/storage/reconcile_job_test.rb b/test/jobs/storage/reconcile_job_test.rb index 038dfd89f..088999aa8 100644 --- a/test/jobs/storage/reconcile_job_test.rb +++ b/test/jobs/storage/reconcile_job_test.rb @@ -45,4 +45,29 @@ class Storage::ReconcileJobTest < ActiveJob::TestCase test "job queued to backend queue" do assert_equal "backend", Storage::ReconcileJob.new.queue_name end + + test "job has concurrency limit by owner" do + job = Storage::ReconcileJob.new(@board) + # limits_concurrency is a Solid Queue feature + # Just verify the job can be instantiated and has the correct queue + assert_equal "backend", job.queue_name + end + + test "job raises ReconcileAborted when reconcile fails" do + # Use perform_now with a fresh board that we can stub + board = @account.boards.create!(name: "Abort Test Board", creator: users(:david)) + + # Prepend a module to intercept reconcile_storage + intercept = Module.new do + def reconcile_storage + false + end + end + board.singleton_class.prepend(intercept) + + assert_raises Storage::ReconcileJob::ReconcileAborted do + # Call perform directly to avoid serialization + Storage::ReconcileJob.new.perform(board) + end + end end diff --git a/test/models/storage/entry_test.rb b/test/models/storage/entry_test.rb index 0a7665532..b0c08c3ce 100644 --- a/test/models/storage/entry_test.rb +++ b/test/models/storage/entry_test.rb @@ -50,12 +50,14 @@ class Storage::EntryTest < ActiveSupport::TestCase end end - test "record accepts _id params for after_destroy_commit snapshots" do + test "record works with destroyed records (destroyed? check)" do + # Simulate a destroyed record - .id still works after destroy + @card.destroy + entry = Storage::Entry.record \ - account_id: @account.id, - board_id: @board.id, - recordable_type: "Card", - recordable_id: @card.id, + account: @account, + board: @board, + recordable: @card, delta: 2048, operation: "detach" @@ -110,31 +112,36 @@ class Storage::EntryTest < ActiveSupport::TestCase end end - test "record does not enqueue job when account is deleted" do - # The graceful handling is that find_by returns nil, so no job is enqueued - # for a non-existent account. We can't test with a fake ID due to FK constraints, - # but we can verify the find_by behavior by stubbing. - Account.stubs(:find_by).returns(nil) + test "record skips entirely when account is destroyed" do + # No need to track storage for deleted accounts + @account.destroy - assert_no_enqueued_jobs only: Storage::MaterializeJob do - Storage::Entry.record \ + assert_no_difference "Storage::Entry.count" do + result = Storage::Entry.record \ account: @account, delta: 1024, operation: "attach" + + assert_nil result end end - test "record does not enqueue board job when board is deleted" do - Board.stubs(:find_by).returns(nil) + test "record does not enqueue board job when board is destroyed" do + @board.destroy - # Account job still enqueued, but board job is not - entry = Storage::Entry.record \ - account: @account, - board: @board, - delta: 1024, - operation: "attach" + # Account job still enqueued, but destroyed board skips its job + jobs = [] + assert_enqueued_with job: Storage::MaterializeJob, args: [ @account ] do + Storage::Entry.record \ + account: @account, + board: @board, + delta: 1024, + operation: "attach" + end - assert_not_nil entry + # Verify board job was NOT enqueued + board_jobs = enqueued_jobs.select { |j| j["arguments"].include?(@board.to_global_id.to_s) } + assert_empty board_jobs end test "entries belong to account" do diff --git a/test/models/storage/no_reuse_test.rb b/test/models/storage/no_reuse_test.rb new file mode 100644 index 000000000..5af07511f --- /dev/null +++ b/test/models/storage/no_reuse_test.rb @@ -0,0 +1,88 @@ +require "test_helper" + +class Storage::NoReuseTest < ActiveSupport::TestCase + setup do + Current.session = sessions(:david) + @account = accounts("37s") + Current.account = @account # Ensure blobs get correct account_id + @board = @account.boards.create!(name: "Test", creator: users(:david)) + end + + # No-reuse validation + # NOTE: For persisted records, ActiveStorage::Attached::One#attach raises + # ActiveRecord::RecordNotSaved when validation fails. + + test "rejects attaching blob that already has tracked attachment" do + blob = ActiveStorage::Blob.create_and_upload! \ + io: StringIO.new("x" * 1000), + filename: "test.png", + content_type: "image/png" + + # First attachment succeeds + card1 = @board.cards.create!(title: "Card 1", creator: users(:david)) + card1.image.attach(blob) + assert card1.image.attached? + + # Second attachment of same blob fails + card2 = @board.cards.create!(title: "Card 2", creator: users(:david)) + assert_raises ActiveRecord::RecordNotSaved do + card2.image.attach(blob) + end + + # Verify only one attachment exists for this blob + assert_equal 1, ActiveStorage::Attachment.where(blob_id: blob.id).count + 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)) + + # Blob created in @account context + blob = ActiveStorage::Blob.create_and_upload! \ + io: StringIO.new("x" * 1000), + filename: "test.png", + content_type: "image/png" + + card = other_board.cards.create!(title: "Card", creator: users(:david)) + assert_raises ActiveRecord::RecordNotSaved do + card.image.attach(blob) + end + + # Verify attachment was not created (blob account doesn't match record account) + assert_not card.reload.image.attached? + end + + test "allows attaching blob to untracked record type" do + file = file_fixture("moon.jpg") + blob = ActiveStorage::Blob.create_and_upload! \ + io: file.open, + filename: "avatar.jpg", + content_type: "image/jpeg" + + # User avatar is not a tracked record type + user = users(:david) + user.avatar.attach(blob) + + # Should succeed - avatars are not storage-tracked + assert user.avatar.attached? + end + + test "allows multiple attachments of same blob to untracked record types" do + file = file_fixture("moon.jpg") + blob = ActiveStorage::Blob.create_and_upload! \ + io: file.open, + filename: "avatar.jpg", + content_type: "image/jpeg" + + # First attachment to untracked (avatar) + user1 = users(:david) + user1.avatar.attach(blob) + assert user1.avatar.attached? + + # Second attachment to untracked (another avatar) should work + # since no-reuse only checks tracked contexts + user2 = users(:jz) + user2.avatar.attach(blob) + assert user2.avatar.attached? + end +end diff --git a/test/models/storage/totaled_test.rb b/test/models/storage/totaled_test.rb index f53ce1a89..4ea91bef1 100644 --- a/test/models/storage/totaled_test.rb +++ b/test/models/storage/totaled_test.rb @@ -273,4 +273,85 @@ class Storage::TotaledTest < ActiveSupport::TestCase assert_not_nil entry assert_equal(-5000, entry.delta) end + + test "reconcile_storage aborts when entry added during scan" do + board = @account.boards.create!(name: "Test Board", creator: users(:david)) + card = board.cards.create!(title: "Test Card", creator: users(:david)) + card.image.attach io: StringIO.new("x" * 1000), filename: "test.png", content_type: "image/png" + + # Delete entry to create drift + Storage::Entry.where(board: board).delete_all + + # Intercept calculate_real_storage_bytes to insert a new entry mid-scan, + # faithfully simulating a concurrent upload that changes the cursor + board.define_singleton_method(:calculate_real_storage_bytes) do + Storage::Entry.create!( + account_id: account.id, + board_id: id, + delta: 500, + operation: "attach" + ) + super() + end + + # Should abort and return false without creating reconcile entry + assert_no_difference "Storage::Entry.where(operation: 'reconcile').count" do + result = board.reconcile_storage + assert_equal false, result + end + end + + test "reconcile_storage returns true on success" do + board = @account.boards.create!(name: "Test Board", creator: users(:david)) + + result = board.reconcile_storage + + assert_equal true, result + end + + + # ensure_storage_total race safety + + test "ensure_storage_total handles concurrent creation" do + @account.storage_total&.destroy + + threads = 3.times.map do + Thread.new do + ActiveRecord::Base.connection_pool.with_connection do + @account.bytes_used_exact + end + end + end + + threads.each(&:join) + assert_equal 1, Storage::Total.where(owner: @account).count + end + + + # per-attachment reconcile + + test "reconcile counts each attachment separately" do + board = @account.boards.create!(name: "Test", creator: users(:david)) + + # Create 3 distinct blobs (one per card) - no reuse + file = file_fixture("moon.jpg") + expected_bytes = file.size + + 3.times do |i| + blob = ActiveStorage::Blob.create_and_upload! \ + io: file.open, + filename: "image_#{i}.jpg", + content_type: "image/jpeg" + + embed = ActionText::Attachment.from_attachable(blob).to_html + board.cards.create!(title: "Card #{i}", description: "

#{embed}

", creator: users(:david)) + end + + Storage::Entry.where(board: board).delete_all + board.reconcile_storage + + entry = Storage::Entry.find_by(board: board, operation: "reconcile") + # 3 attachments x file_size bytes + assert_equal expected_bytes * 3, entry.delta + end end diff --git a/test/models/storage/tracked_test.rb b/test/models/storage/tracked_test.rb index 029329792..f311e963e 100644 --- a/test/models/storage/tracked_test.rb +++ b/test/models/storage/tracked_test.rb @@ -18,6 +18,32 @@ class Storage::TrackedTest < ActiveSupport::TestCase assert_equal 1024, @card.storage_bytes end + test "storage_bytes includes rich text embeds" do + blob = ActiveStorage::Blob.create_and_upload! \ + io: file_fixture("moon.jpg").open, + filename: "embed.jpg", + content_type: "image/jpeg" + + embed_html = ActionText::Attachment.from_attachable(blob).to_html + @card.update!(description: "

Content with #{embed_html}

") + + assert_equal blob.byte_size, @card.storage_bytes + end + + test "storage_bytes sums direct attachments and rich text embeds" do + @card.image.attach io: StringIO.new("x" * 1024), filename: "test.png", content_type: "image/png" + + blob = ActiveStorage::Blob.create_and_upload! \ + io: file_fixture("moon.jpg").open, + filename: "embed.jpg", + content_type: "image/jpeg" + + embed_html = ActionText::Attachment.from_attachable(blob).to_html + @card.update!(description: "

Content with #{embed_html}

") + + assert_equal 1024 + blob.byte_size, @card.storage_bytes + end + test "board transfer creates transfer_out entry for old board" do @card.image.attach io: StringIO.new("x" * 2048), filename: "test.png", content_type: "image/png" old_board_id = @card.board_id @@ -59,6 +85,115 @@ class Storage::TrackedTest < ActiveSupport::TestCase assert_equal initial_count, final_count end + test "board transfer moves card description embeds" do + blob = ActiveStorage::Blob.create_and_upload! \ + io: file_fixture("moon.jpg").open, + filename: "card_embed.jpg", + content_type: "image/jpeg" + + embed_html = ActionText::Attachment.from_attachable(blob).to_html + @card.update!(description: "

Desc with image #{embed_html}

") + + old_board_id = @card.board_id + + assert_difference -> { Storage::Entry.where(operation: "transfer_out", recordable: @card).count }, +1 do + assert_difference -> { Storage::Entry.where(operation: "transfer_in", recordable: @card).count }, +1 do + @card.update!(board: @board2) + end + end + + transfer_out = Storage::Entry.where(operation: "transfer_out", recordable: @card).last + transfer_in = Storage::Entry.where(operation: "transfer_in", recordable: @card).last + + assert_equal(-blob.byte_size, transfer_out.delta) + assert_equal old_board_id, transfer_out.board_id + assert_equal blob.byte_size, transfer_in.delta + assert_equal @board2.id, transfer_in.board_id + end + + test "board transfer moves comment embeds" do + blob = ActiveStorage::Blob.create_and_upload! \ + io: file_fixture("moon.jpg").open, + filename: "comment_embed.jpg", + content_type: "image/jpeg" + + embed_html = ActionText::Attachment.from_attachable(blob).to_html + comment = @card.comments.create!(body: "

Comment with image #{embed_html}

") + + old_board_id = @card.board_id + + assert_difference -> { Storage::Entry.where(operation: "transfer_out", recordable: comment).count }, +1 do + assert_difference -> { Storage::Entry.where(operation: "transfer_in", recordable: comment).count }, +1 do + @card.update!(board: @board2) + end + end + + transfer_out = Storage::Entry.where(operation: "transfer_out", recordable: comment).last + transfer_in = Storage::Entry.where(operation: "transfer_in", recordable: comment).last + + assert_equal(-blob.byte_size, transfer_out.delta) + assert_equal old_board_id, transfer_out.board_id + assert_equal blob.byte_size, transfer_in.delta + assert_equal @board2.id, transfer_in.board_id + end + + test "board transfer moves card image and description embed together" do + @card.image.attach io: StringIO.new("x" * 1024), filename: "test.png", content_type: "image/png" + + blob = ActiveStorage::Blob.create_and_upload! \ + io: file_fixture("moon.jpg").open, + filename: "card_embed.jpg", + content_type: "image/jpeg" + + embed_html = ActionText::Attachment.from_attachable(blob).to_html + @card.update!(description: "

Desc with #{embed_html}

") + + old_board_id = @card.board_id + expected_bytes = 1024 + blob.byte_size + + # One transfer_out and one transfer_in for the card (combined bytes) + assert_difference -> { Storage::Entry.where(operation: "transfer_out", recordable: @card).count }, +1 do + assert_difference -> { Storage::Entry.where(operation: "transfer_in", recordable: @card).count }, +1 do + @card.update!(board: @board2) + end + end + + transfer_out = Storage::Entry.where(operation: "transfer_out", recordable: @card).last + transfer_in = Storage::Entry.where(operation: "transfer_in", recordable: @card).last + + assert_equal(-expected_bytes, transfer_out.delta) + assert_equal expected_bytes, transfer_in.delta + end + + test "board transfer moves multiple comments with embeds" do + blob1 = ActiveStorage::Blob.create_and_upload! \ + io: file_fixture("moon.jpg").open, + filename: "embed1.jpg", + content_type: "image/jpeg" + blob2 = ActiveStorage::Blob.create_and_upload! \ + io: file_fixture("moon.jpg").open, + filename: "embed2.jpg", + content_type: "image/jpeg" + + comment1 = @card.comments.create!(body: "

#{ActionText::Attachment.from_attachable(blob1).to_html}

") + comment2 = @card.comments.create!(body: "

#{ActionText::Attachment.from_attachable(blob2).to_html}

") + + old_board_id = @card.board_id + + # Should create transfer entries for both comments + assert_difference -> { Storage::Entry.where(operation: "transfer_out").count }, +2 do + assert_difference -> { Storage::Entry.where(operation: "transfer_in").count }, +2 do + @card.update!(board: @board2) + end + end + + # Verify each comment's transfer + assert_equal(-blob1.byte_size, Storage::Entry.find_by(operation: "transfer_out", recordable: comment1).delta) + assert_equal blob1.byte_size, Storage::Entry.find_by(operation: "transfer_in", recordable: comment1).delta + assert_equal(-blob2.byte_size, Storage::Entry.find_by(operation: "transfer_out", recordable: comment2).delta) + assert_equal blob2.byte_size, Storage::Entry.find_by(operation: "transfer_in", recordable: comment2).delta + end + test "board transfer net effect on account is zero" do @card.image.attach io: StringIO.new("x" * 1024), filename: "test.png", content_type: "image/png" @@ -116,4 +251,36 @@ class Storage::TrackedTest < ActiveSupport::TestCase assert_equal 1, attachments.count assert_equal @card.image.blob.byte_size, attachments.first.blob.byte_size end + + test "attachments_for_storage includes rich text embeds" do + blob = ActiveStorage::Blob.create_and_upload! \ + io: file_fixture("moon.jpg").open, + filename: "embed.jpg", + content_type: "image/jpeg" + + embed_html = ActionText::Attachment.from_attachable(blob).to_html + @card.update!(description: "

Content with #{embed_html}

") + + attachments = @card.send(:attachments_for_storage) + + assert_equal 1, attachments.count + assert_equal blob.byte_size, attachments.first.blob.byte_size + end + + test "attachments_for_storage includes both direct and rich text attachments" do + @card.image.attach io: StringIO.new("x" * 1024), filename: "test.png", content_type: "image/png" + + blob = ActiveStorage::Blob.create_and_upload! \ + io: file_fixture("moon.jpg").open, + filename: "embed.jpg", + content_type: "image/jpeg" + + embed_html = ActionText::Attachment.from_attachable(blob).to_html + @card.update!(description: "

Content with #{embed_html}

") + + attachments = @card.send(:attachments_for_storage) + + assert_equal 2, attachments.count + assert_equal 1024 + blob.byte_size, attachments.sum { |a| a.blob.byte_size } + end end