Skip to content

Track Active Storage variants in the database #37901

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Dec 6, 2019
Merged

Track Active Storage variants in the database #37901

merged 1 commit into from
Dec 6, 2019

Conversation

georgeclaghorn
Copy link
Contributor

@georgeclaghorn georgeclaghorn commented Dec 6, 2019

Currently, when a variant is requested, we check whether it exists in the service. If it doesn’t, we generate it on-demand and store it for reuse.

The existence check can add unacceptable latency to variant serving. We’ve seen existence checks to third-party services take upwards of 500ms in normal circumstances. Further, checking whether an object exists at a particular key before uploading it triggers eventual consistency in S3:

Amazon S3 provides read-after-write consistency for PUTS of new objects in your S3 bucket in all Regions with one caveat. The caveat is that if you make a HEAD or GET request to the key name (to find if the object exists) before creating the object, Amazon S3 provides eventual consistency for read-after-write.

This means that when a variant is requested for the first time, generated, and stored in S3, subsequent redirects to the variant’s service URL can fail indefinitely thereafter. The result is sporadically broken images.

This PR addresses the above concerns by doing away with the existence check. When a variant is requested for the first time, we generate it, store it, and track it in the application database. Now we can know whether we’ve already generated a variant without making a remote call to the storage service.

@georgeclaghorn georgeclaghorn merged commit 7d0327b into rails:master Dec 6, 2019
@georgeclaghorn georgeclaghorn deleted the activestorage-variant-tracking branch December 6, 2019 18:26
@georgeclaghorn georgeclaghorn added this to the 6.1.0 milestone Dec 6, 2019

def create_or_find_record(image:)
@record =
ActiveRecord::Base.connected_to(role: ActiveRecord::Base.writing_role) do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be desirable to extract this method so apps can override to connect to whatever db role. Probably premature.

@jeremiemv
Copy link

Thanks @georgeclaghorn
This will be a great improvement :)

@simonfranzen
Copy link

simonfranzen commented Jun 8, 2020

Hey. This seems like a very nice feature. I am struggling a bit. Is is merged?
I upgraded from rails 5 to rails 6 recently. I have created the active_storage_variant_records table and enabled it in the config, but it's not storing any entries in the db.

ActiveStorage::VariantWithRecord is also not loaded.

undefined method `variant_records' for #ActiveStorage::Blob:0x00007fbcffe12670

@sedubois
Copy link
Contributor

sedubois commented Jun 8, 2020

@simonfranzen this ships in 6.1
Screenshot 2020-06-08 at 19 26 58

@buhrmi
Copy link

buhrmi commented Jun 11, 2020

Can't wait for this to be released. Will there be a way to include variants when querying the database to prevent N+1 queries?

joshmcarthur added a commit to ackama/nzsl-share that referenced this pull request Jun 15, 2020
Rails currently needs to perform a network lookup to detect if a variant exists
before it can serve the URL. This is fixed in Rails 6.1,
rails/rails#37901.

Until then, we cache the generated URL to make sure that we don't incur a
200-500ms penalty on each sign card
@vizcay
Copy link
Contributor

vizcay commented Oct 27, 2020

Hi @georgeclaghorn, this is great work and pretty much needed when the S3 server goes down as happened a few times to me and others (#39327).

I've quickly tried to monkey patch ActiveStorage on Rails 6.0 to track already created variants and worked but ended up hitting the problem of the N+1 Query as I wasn't able to preload al the variations when loading all the images.

Before investing more time there, there is no way of using this on Rails 6.0 without bundling all rails from master, right?

@Cremz
Copy link

Cremz commented Nov 9, 2020

This is indeed a great improvement, however the N+1 issue is very frustrating. I rather hit the server and check for the images instead of doing the preloaded query, then 3 x n+1 calls for each variant. (even worse when the model has multiple attached images). Any progress on a fix for this?

@rept
Copy link

rept commented Dec 2, 2020

@georgeclaghorn great addition. Am I right to assume that for existing variants (so already created before this new feature) the check will do done as follows:

  • check in DB if variant exists
  • if not exists, check in service
  • if found there: insert variant record as well
  • if not found there: create variant as usual

Another question: is the following intended?

After saving the image I do this to create a resized version:

device_capture.source_image.attach(io: image, filename: 'file.jpg', content_type: "image/jpg")
device_capture.source_image.analyze
device_capture.save
device_capture.source_image.variant(resize: "256x144^").processed

This version gets created and uploaded fine (see log below). However when I look in the table active_storage_variant_records there are no records created for this? I have to mention, I do this right after saving the initial image.

INFO -- : GCS Storage (166.2ms) Uploaded file to key: variants/bqybptq4psz72e8cirbktuou0fu9/88b54084c3b692042cf443b2ff6a221419d09ab4b6dfc1df7bef5f8b85ef0411

@ghiculescu
Copy link
Member

For those concerned about n+1s, please give #40842 a try and let me know if it improves things for you.

goulvench added a commit to goulvench/database_consistency that referenced this pull request Dec 30, 2020
Since Rails 6.1, ActiveStorage stores variants metadata in a dedicated table (see [Pull Request 37901](rails/rails#37901) for full explanation).
This addition ensures that the `install` command also adds the required configuration to prevent false positives caused by the `active_storage_variant_records` table.

Here are the failure messages displayed if the table is not ignored:
```
fail ActiveStorage::VariantRecord variation_digest column is required in the database but do not have presence validator
fail ActiveStorage::VariantRecord index_active_storage_variant_records_uniqueness index is unique in the database but do not have uniqueness validator
```
@Laykou
Copy link

Laykou commented Mar 9, 2021

How does it work with existing variants in db? After deploying this to production will existing variants still work as addressed above? #37901 (comment)

  • check in DB if variant exists
  • if not exists, check in service
  • if found there: insert variant record as well
  • if not found there: create variant as usual

@louim
Copy link
Contributor

louim commented Mar 9, 2021

@Laykou, yes, existing variants will create a new record in the database the first time they are accessed and will skip the service exist call after that.

waiting-for-dev added a commit to nebulab/solidus that referenced this pull request Apr 22, 2021
Using `load_defaults` for Rails 6.1 (see 31712c2) makes
`active_storage.track_variants` true by default.

Here we remove the overridden setting and adapt our test to invalidate
the blob with the new default in place.

See also rails/rails#37901
waiting-for-dev added a commit to nebulab/solidus that referenced this pull request Apr 30, 2021
Using `load_defaults` for Rails 6.1 (added to the dummy app in a recent
commit) makes `active_storage.track_variants` true by default.

Here we remove the overridden setting and adapt our test to invalidate
the blob with the new default in place.

See also rails/rails#37901
ghiculescu added a commit to ghiculescu/rails that referenced this pull request Jun 3, 2021
…ds on an attachment at once

Currently Active Storage [variant tracking](rails#37901) runs a query for each attachment to check for a variant. Regular Rails n+1 prevention techniques (`includes`) cannot prevent this.

This PR adds `with_all_variant_records`, as well as allowing `includes` to work as expected on Active Storage attachments.

```ruby
user.vlogs.with_all_variant_records.map do |vlog|
  vlog.representation(resize: "100x100").processed
end
```

Fixes several of the comments in rails#37901

In implementing this, I borrowed heavily from the style of rails#39397
@hong-godaddy
Copy link

Are we able to backport this to 5.2?

@louim
Copy link
Contributor

louim commented Jul 30, 2021

Hey @hong-godaddy, features are not backported to earlier versions of Rails.

@KSH-code
Copy link

KSH-code commented Oct 5, 2021

S3 has supported Strong consistency model now from Dec. 2020.

Is it possible to change default as false?

@tbhockey
Copy link

tbhockey commented Jan 13, 2022

First of all, thank you for this feature 🙏

I'm coming from Rails 5 and have upgraded to 6.1 and have enabled this with config.active_storage.track_variants = true and while it's working beautifully, but we are getting quite a few broken images. I deleted all VariantRecord objects to allow them to rebuild, but strangely this fixed some images and now others are broken. Any idea how to properly resolve this?

@matthee
Copy link
Contributor

matthee commented Feb 1, 2022

Hi @tbhockey! I've experienced a similar problem now. Here is how i solved it:

tl;dr run ActiveStorage::VariantRecord.destroy_all

First, I tried to delete my variants like so:

ActiveStorage::Attachment.where(record_type: "ActiveStorage::VariantRecord").destroy_all

This looks fine at first glance. However, ActiveStorage actually creates ActiveStorage::VariantRecord records for each variant in the database. When you then browse to a page, where the variant is requested, ActiveStorage looks for the corresponding ActiveStorage::VariantRecord record in the database. If it is found, rails figures that the variant must also exist in the storage service (which it does not), leading to a broken image tag in the browser.

In order to fix this problem the ActiveStorage::VariantRecord records must also be removed from the database.

Running the following command in the rails console finally fixed my problem and rails happily regenerates all my variants on the fly:

ActiveStorage::VariantRecord.destroy_all

@tbhockey
Copy link

tbhockey commented Feb 1, 2022

Thanks for the reply @matthee . I actually tried this as well, but for me, what happened was after doing that, different images were broken and others were fixed 😆

We ended up just having to go through and manually delete the Variants for any of the broken images. Since then, everything has been fine. But absolutely no clue what was causing that.

rmparr pushed a commit to rmparr/solidus that referenced this pull request Jun 1, 2022
Using `load_defaults` for Rails 6.1 (added to the dummy app in a recent
commit) makes `active_storage.track_variants` true by default.

Here we remove the overridden setting and adapt our test to invalidate
the blob with the new default in place.

See also rails/rails#37901
rmparr pushed a commit to rmparr/solidus that referenced this pull request Jun 1, 2022
Using `load_defaults` for Rails 6.1 (added to the dummy app in a recent
commit) makes `active_storage.track_variants` true by default.

Here we remove the overridden setting and adapt our test to invalidate
the blob with the new default in place.

See also rails/rails#37901
jyoun-godaddy pushed a commit to jyoun-godaddy/activestorage that referenced this pull request Jul 5, 2022
…ds on an attachment at once

Currently Active Storage [variant tracking](rails/rails#37901) runs a query for each attachment to check for a variant. Regular Rails n+1 prevention techniques (`includes`) cannot prevent this.

This PR adds `with_all_variant_records`, as well as allowing `includes` to work as expected on Active Storage attachments.

```ruby
user.vlogs.with_all_variant_records.map do |vlog|
  vlog.representation(resize: "100x100").processed
end
```

Fixes several of the comments in rails/rails#37901

In implementing this, I borrowed heavily from the style of rails/rails#39397
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.