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