Expire caching when a download fail while proxying in ActiveStorage

Fix rails#51284

Both Proxy controllers in Active Storage set the caching headers early
before streaming.

In some instances (see rails#51284), it is possible for the file
download (from the service to server) to fail before we send the first
byte to the client (response not yet committed).

In those instances, this change would invalidate the cache and return
a better response status before closing the stream.

Author: JoeDupuis

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