Skip to content
This repository has been archived by the owner on Oct 5, 2018. It is now read-only.

Prevent deleting processing image while attachment is processing #137

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cgunther
Copy link

@cgunther cgunther commented Mar 3, 2015

On destroying a model, Paperclip will delete any styles for which exists? returns true. exists? checked the path, which would point to the processing image while the attachment was processing when processing_image_url is set. This leads to Paperclip deleting the
processing image, thinking it is the style for the attachment.

Given:

class Photos < ActiveRecord::Base
  has_attached_file :image, styles: { thumbnail: '100x' }, only_process: [:original]
  process_in_background :image, only_process: [:thumbnail], processing_image_url: '/images/:class/processing/:attachment/:style.gif'
end

When deleting a photo that was not yet processed, you'd see this in your logs as Paperclip deleted both the original (correctly) and the thumbnail processing image (incorrectly):

[paperclip] deleting /Users/chrisgunther/Sites/sample/public/system/photos/images/000/000/185/original/brick20150303-42943-1bubzim.jpeg
[paperclip] deleting /Users/chrisgunther/Sites/sample/public/images/photos/processing/images/thumbnail.gif

This patch overrides Paperclip's exists? method so that if the given style is processing, exists? will always return false, thus preventing Paperclip from trying to delete the processing image. This is not backwards compatible as exists? previously returned true even if the
style was not yet processed.

Previously:

photo = Photo.create(image: File.open('/path/to/image')
photo.image.exists?(:original) # => true
photo.image.exists?(:thumbnail) # => true

Now:

photo = Photo.create(image: File.open('/path/to/image')
photo.image.exists?(:original) # => true
photo.image.exists?(:thumbnail) # => false
# Process styles
photo.image.exists?(:thumbnail) # => true

This may still leave some orphaned files as if the model is destroyed while the processing job is currently running, no styles will be deleted, but some styles may have already been written to disk as the job runs.

While it's a small window to delete the model in before the background job finishes running, it's a pretty big issue that the processing images that you think are safe in your app may be deleted and will now 404 on future requests to them until you deploy again, adding the processing images, only for them to be deleted again when someone deletes the model before the background job finishes.

On destroying a model, Paperclip will delete any styles for which
exists? returns true. exists? checked the path, which would point to the
processing image while the attachment was processing when
processing_image_url is set. This leads to Paperclip deleting the
processing image, thinking it is the style for the attachment.

This patch overrides Paperclip's exists? method so that if the given
style is processing, exists? will always return false, thus preventing
Paperclip from trying to delete the processing image. This is not
backwards compatible as exists? previously returned true even if the
style was not yet processed.

This may still leave some orphaned files as if the model is destroyed
while the processing job is currently running, no styles will be
deleted, but some styles may have already been written to disk as the
job runs.
@ScotterC
Copy link
Collaborator

ScotterC commented Sep 8, 2015

hey @cgunther this is really useful! Would you mind taking a look at the rails 3.2 failures? This is telling me maybe we should stop supporting Rails < 4 but if it's an easy fix it'd be welcome

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants