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
This commit is contained in:
Jeremy Daer
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
+12
-16
@@ -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
|
||||
|
||||
@@ -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
|
||||
@@ -8,7 +8,21 @@
|
||||
# Run via Kamal:
|
||||
# kamal app exec -d <stage> -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
@@ -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: "<p>#{embed}</p>", 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
|
||||
|
||||
@@ -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: "<p>Content with #{embed_html}</p>")
|
||||
|
||||
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: "<p>Content with #{embed_html}</p>")
|
||||
|
||||
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: "<p>Desc with image #{embed_html}</p>")
|
||||
|
||||
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: "<p>Comment with image #{embed_html}</p>")
|
||||
|
||||
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: "<p>Desc with #{embed_html}</p>")
|
||||
|
||||
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: "<p>#{ActionText::Attachment.from_attachable(blob1).to_html}</p>")
|
||||
comment2 = @card.comments.create!(body: "<p>#{ActionText::Attachment.from_attachable(blob2).to_html}</p>")
|
||||
|
||||
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: "<p>Content with #{embed_html}</p>")
|
||||
|
||||
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: "<p>Content with #{embed_html}</p>")
|
||||
|
||||
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
|
||||
|
||||
Reference in New Issue
Block a user