Merge pull request #6822 from chef/lcg/remote_file_cleanup_tempfiles

RemoteFile: unlink tempfile when using cache control shows unchanged

2 files changed, 14 insertions, 11 deletions

diff --git a/lib/chef/provider/remote_file/http.rb b/lib/chef/provider/remote_file/http.rb
index 4732253e5b..8dfa84ee2a 100644
--- a/lib/chef/provider/remote_file/http.rb
+++ b/lib/chef/provider/remote_file/http.rb
@@ -61,17 +61,22 @@ class Chef
 
         def fetch
           http = Chef::HTTP::Simple.new(uri, http_client_opts)
-          tempfile = Chef::FileContentManagement::Tempfile.new(@new_resource).tempfile
+          orig_tempfile = Chef::FileContentManagement::Tempfile.new(@new_resource).tempfile
           if want_progress?
-            tempfile = http.streaming_request_with_progress(uri, headers, tempfile) do |size, total|
+            tempfile = http.streaming_request_with_progress(uri, headers, orig_tempfile) do |size, total|
               events.resource_update_progress(new_resource, size, total, progress_interval)
             end
           else
-            tempfile = http.streaming_request(uri, headers, tempfile)
+            tempfile = http.streaming_request(uri, headers, orig_tempfile)
           end
           if tempfile
             update_cache_control_data(tempfile, http.last_response)
             tempfile.close
+          else
+            # cache_control shows the file is unchanged, so we got back nil from the streaming_request above, and it is
+            # now our responsibility to unlink the tempfile we created
+            orig_tempfile.close
+            orig_tempfile.unlink
           end
           tempfile
         end
diff --git a/spec/unit/provider/remote_file/http_spec.rb b/spec/unit/provider/remote_file/http_spec.rb
index ec9c0cf2e3..e1f74bd35a 100644
--- a/spec/unit/provider/remote_file/http_spec.rb
+++ b/spec/unit/provider/remote_file/http_spec.rb
@@ -185,14 +185,12 @@ describe Chef::Provider::RemoteFile::HTTP do
       expect(Chef::HTTP::Simple).to receive(:new).with(*expected_http_args).and_return(rest)
     end
 
-    describe "and the request does not return new content" do
-
-      it "should return a nil tempfile for a 304 HTTPNotModifed" do
-        # Streaming request returns nil for 304 errors
-        expect(rest).to receive(:streaming_request).with(uri, {}, tempfile).and_return(nil)
-        expect(fetcher.fetch).to be_nil
-      end
-
+    it "should clean up the tempfile, and return a nil when streaming_request returns nil" do
+      # Streaming request returns nil for a 304 not modified (etags / last-modified)
+      expect(rest).to receive(:streaming_request).with(uri, {}, tempfile).and_return(nil)
+      expect(tempfile).to receive(:close)
+      expect(tempfile).to receive(:unlink)
+      expect(fetcher.fetch).to be_nil
     end
 
     context "with progress reports" do