Skip to content
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

Corrupted Active Storage attachments #4017

Closed
kennyadsl opened this issue Apr 2, 2021 · 3 comments
Closed

Corrupted Active Storage attachments #4017

kennyadsl opened this issue Apr 2, 2021 · 3 comments
Labels
type:bug Error, flaw or fault
Milestone

Comments

@kennyadsl
Copy link
Member

Hey guys!
I have an issue using ActiveStorageAttachment for product images.
I'm using an S3-compatible service for storing attachments, and it's working ok. But if the service returns an error (it is down at the moment the user attempts to upload an image or there's a permission error, etc), a product image record and ActiveStorage blob record are created anyway despite the image processing error. So it produces a corrupted state of the system. And then any code which calls .url(style) on this image gets an ActiveStorage::FileNotFoundError exception, because it tries to run image processing to create the requested image variant, but it can't find the original image because its upload never finished (it happens on any page which needs to display the image).
Ideally, I would like it to be "all or nothing", so the database records get committed only after the storage service responded with a success.
Is it a problem of how ActiveStorage works itself or is it a Solidus wrapper's one? How do I prevent the situation when one failed image upload breaks down the whole store (you can't even delete the corrupted image, because the admin product page is also broken)?

Originally posted by @ok32 in #3986

@kennyadsl kennyadsl added the type:bug Error, flaw or fault label Apr 2, 2021
@kennyadsl kennyadsl added this to the 3.0.0 milestone Apr 2, 2021
@kennyadsl
Copy link
Member Author

I had another issue uploading a not supported file type (.heic), not sure it's related but I also had to remove the image from the database to fix it. To reproduce:

rails new store --skip-javascript
cd store
bundle add solidus -v 3.0.0.rb2
bin/rails generate solidus:install

Login into the admin and upload a .heic file in any product. Reload the page to see the ActiveStorage::InvariableError.

@cpfergus1
Copy link
Contributor

cpfergus1 commented Apr 5, 2021

I was able recreate this issue by:

  1. creating a new application
  2. add gem 'solidus', path: '/Users/_user_/solidus-3 and:
$ bundle install
$ bin/rails generate solidus:install
  1. setup s3 bucket at AWS (https://docs.aws.amazon.com/AmazonS3/latest/userguide/creating-bucket.html)
  2. change root/config/storage.yml to include s3 bucket:
  service: S3
  access_key_id: <%= Rails.application.credentials.dig(:aws, :access_key_id) %>
  secret_access_key: <%= Rails.application.credentials.dig(:aws, :secret_access_key) %>
  region: us-west-2
  bucket: _bucket_
  1. change root/config/environments/development.rb config.active_storage.service = :amazon
  2. slightly change your AWS credentials prior to booting up your server by commenting out part of your ID
  3. Boot up server, and navigate to admin page to try and upload a photo - should receive uploading error.
  4. reload webpage and get first error Aws::S3::Errors::InvalidAccessKeyId
  5. reload server with correct credentials and reload page to receive the desired error ActiveStorage::FileNotFoundError in Spree::Admin::Images#index

The issue appears to be with ActiveStorage as it queues the upload of the image after the objects are created and when it fails to upload, it does not purge the existence of the image and the created objects. The image and attachment persist with an ActiveStorage record still found in the database. Subsequent calls for the object expect a file to be returned from the server and solidus currently does not handle the exception for missing file.

Initial remedy provided by the original poster was an exception handle in ActiveStorageAttachment that appended a revised url method:

    def url(style = nil)
      super(style)
    rescue ActiveStorage::FileNotFoundError
      '/images/corrupted-image.png'
    end

This solution does not rectify the creation of an image or attachment when there is a failure to upload, and only bypasses an exception if the the image cannot be found on third part storage.

Ideally we would want to prevent the object from ever being created should the image fail to upload. This might be difficult to accomplish based on the way the the objects and jobs are created. Therefore, we could also potentially run a validation ActiveStorageAdapter or implement a post processing job to clean up Images with failed uploads.

@cpfergus1
Copy link
Contributor

Stack trace for further investigation

StackTrace.txt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Error, flaw or fault
Projects
None yet
Development

No branches or pull requests

2 participants