[Bug] File not promoted on permanent storage if reload the model inside a transaction

[johnvuko]

Brief Description

Hello, Inside a transaction, if a create a model then reload it with the find method, the file is not promoted on permanent storage (store) and stay in cache. It doesn't happen if I call reload on the model or without the transaction.

This bug doesn't seems to be related with #366

Expected behavior

The storage should be store.

Actual behavior

The storage is cache.

Simplest self-contained example code to demonstrate issue

require 'active_record'
require 'shrine'
require 'shrine/storage/file_system'
require 'tmpdir'
require 'down'

Shrine.storages = {
  cache: Shrine::Storage::FileSystem.new(File.join(Dir.tmpdir, 'cache')),
  store: Shrine::Storage::FileSystem.new(File.join(Dir.tmpdir, 'uploads'))
}

Shrine.plugin :determine_mime_type
Shrine.plugin :activerecord

ActiveRecord::Base.establish_connection(adapter: 'sqlite3', database: ':memory:')
ActiveRecord::Base.connection.create_table(:products) { |t| t.json :image_data }

class Product < ActiveRecord::Base
  include Shrine::Attachment.new(:image)
end

product = nil

ActiveRecord::Base.transaction do
  product = Product.create!
  product = Product.find(product.id) # without this line or without the transaction it works

  params = {
    image: Down.download('https://dummyimage.com/600x400/000/fff')
  }

  product.update!(params)
end

puts product.image.storage_key # return `cache` instead of `store`

System configuration

Ruby: 3.2.2 Shrine: 3.5.0 Ruby On Rails: 7.0.7.2

[hms]

The sample code looks like a race condition between the Shrine after_commit processing used for promotion and update operation. I'm pretty sure Shrine logs under those circumstances. Do you see anything in the logs?

While the sample code demonstrates a problem, what's the business use-case for that workflow? It might be easier to help with where to plug into Shrines multitude of hooks to accomplish your goals if I understood your requirements.

[johnvuko]

The logger show:

Metadata (6ms) – {:storage=>:cache, :io=>Tempfile, :uploader=>Shrine}
Upload (0ms) – {:storage=>:cache, :location=>"54fb171c65dae520870092d5f73eca40", :io=>Tempfile, :upload_options=>{}, :uploader=>Shrine}

My use case is in a large Rails application, we find a way but finding the source of the issue took some time. I would prefer an exception than having this behavior. I thought the problem with hooks in Rails models was long time gone.

[hms]

@jonathantribouharet

A couple of things:

1. The after_commit issue with Shrine classes that include an Attacher is alive and well. I just lost about a week tracking down an issue in my apps workflow. It's documented at https://shrinerb.com/docs/plugins/activerecord#after-save), but I also assumed that the bug was cleared on the Rails side and forgot about it.
2. I'm not so sure about my first, somewhat off the cuff answer about race conditions between the commit and update given they are in the same transaction. But I wouldn't be surprised about an AR object changing from unsaved to saved within a transaction throwing a wrench into the works somewhere between Rails and Shrine.
3. I know your code above was just to demonstrate the problem, but I'm confused by an Application workflow that would result in the two step process of Creating and then Updating within the same transaction -- from a database perspective it's usually a bad practice to hold transactions open for any longer than absolutely necessary, and that workflow suggests a complex series of steps before commit. I wonder if the workflow could be changed from Create -> Update to New -> Save?
4. To address your "safety net" comment -- Exception vs. allowing the promotion to fail, I'm sure it would be just a couple of lines in promotion block to get that exception. But even better, is should be just a couple of lines in one of the various hooks that Shrine exposes to detect the case where the promotion hasn't occurred and to reset the object to the correct state such that it will promote.

[janko]

This is a bug in Active Record. It only calls the after_create_commit hook on the first model instance, but not the after_update_commit hook on the second model instance, which causes promotion of the attachment to permanent storage to be skipped.

require "active_record"

ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.connection.create_table(:products) { |t| t.string :name }

class Product < ActiveRecord::Base
  after_create_commit { puts "after_create_commit" }
  after_update_commit { puts "after_create_commit" }
end

ActiveRecord::Base.transaction do
  product = Product.create!(name: "Foo")
  product = Product.find(product.id)
  product.update!(name: "Bar")
end

# OUTPUT:
# after_create_commit

I thought the problem with hooks in Rails models was long time gone.

Problems with Active Record's transaction callbacks will likely never entirely go away, because the way they're implemented is really messy, so it's unlikely to ever produce reliable behavior. I wrote more about it here.

In Sequel, transaction hooks are implemented cleanly, most importantly they have no knowledge of models. When I modify your example to use Sequel instead of Active Record, it produces the expected result.

So, your only option is to get it fixed in Active Record, or rewrite your code to avoid running into this edge case.