Merge pull request rails#51288 from JoeDupuis/stop-caching-on-failure-activestorage-proxy-controller

Expire caching when a download fail while proxying in ActiveStorage

Author: tenderlove

activestorage/app/controllers/concerns/active_storage/streaming.rb

@@ -61,6 +61,15 @@ def send_blob_stream(blob, disposition: nil) # :doc:
         blob.download do |chunk|
           stream.write chunk
         end
+      rescue ActiveStorage::FileNotFoundError
+        expires_now
+        head :not_found
+      rescue
+        # Status and caching headers are already set, but not commited.
+        # Change the status to 500 manually.
+        expires_now
+        head :internal_server_error
+        raise
       end
     end
 end

activestorage/test/controllers/blobs/proxy_controller_test.rb

@@ -16,6 +16,32 @@ class ActiveStorage::Blobs::ProxyControllerTest < ActionDispatch::IntegrationTes
     assert_equal "max-age=3155695200, public", response.headers["Cache-Control"]
   end
 
+  test "invalidates cache and returns a 404 if the file is not found on download" do
+    blob = create_file_blob(filename: "racecar.jpg")
+    mock_download = lambda do |_|
+      raise ActiveStorage::FileNotFoundError.new "File still uploading!"
+    end
+    blob.service.stub(:download, mock_download) do
+      get rails_storage_proxy_url(blob)
+    end
+    assert_response :not_found
+    assert_equal "no-cache", response.headers["Cache-Control"]
+  end
+
+
+  test "invalidates cache and returns a 500 if an error is raised on download" do
+    blob = create_file_blob(filename: "racecar.jpg")
+    mock_download = lambda do |_|
+      raise StandardError.new "Something is not cool!"
+    end
+    blob.service.stub(:download, mock_download) do
+      get rails_storage_proxy_url(blob)
+    end
+    assert_response :internal_server_error
+    assert_equal "no-cache", response.headers["Cache-Control"]
+  end
+
+
   test "forcing Content-Type to binary" do
     get rails_storage_proxy_url(create_blob(content_type: "text/html"))
     assert_equal "application/octet-stream", response.headers["Content-Type"]

activestorage/test/controllers/representations/proxy_controller_test.rb

@@ -2,6 +2,7 @@
 
 require "test_helper"
 require "database/setup"
+require "minitest/mock"
 
 class ActiveStorage::Representations::ProxyControllerWithVariantsTest < ActionDispatch::IntegrationTest
   setup do
@@ -84,6 +85,39 @@ class ActiveStorage::Representations::ProxyControllerWithVariantsWithStrictLoadi
     assert_match(/^inline/, response.headers["Content-Disposition"])
     assert_equal @blob.variant(@transformations).download, response.body
   end
+
+
+  test "invalidates cache and returns a 404 if the file is not found on download" do
+    # This mock requires a pre-processed variant as processing the variant will call to download
+    mock_download = lambda do |_|
+      raise ActiveStorage::FileNotFoundError.new "File still uploading!"
+    end
+
+    @blob.service.stub(:download, mock_download) do
+      get rails_blob_representation_proxy_url(
+        filename: @blob.filename,
+        signed_blob_id: @blob.signed_id,
+        variation_key: ActiveStorage::Variation.encode(@transformations))
+    end
+    assert_response :not_found
+    assert_equal "no-cache", response.headers["Cache-Control"]
+  end
+
+  test "invalidates cache and returns a 500 if the an error is raised on download" do
+    # This mock requires a pre-processed variant as processing the variant will call to download
+    mock_download = lambda do |_|
+      raise StandardError.new "Something is not cool!"
+    end
+
+    @blob.service.stub(:download, mock_download) do
+      get rails_blob_representation_proxy_url(
+        filename: @blob.filename,
+        signed_blob_id: @blob.signed_id,
+        variation_key: ActiveStorage::Variation.encode(@transformations))
+    end
+    assert_response :internal_server_error
+    assert_equal "no-cache", response.headers["Cache-Control"]
+  end
 end
 
 class ActiveStorage::Representations::ProxyControllerWithPreviewsTest < ActionDispatch::IntegrationTest