clean up deleted files in cookbooks we synch

this also fixes bugs where dotfiles would be missed
in loops that search through the file cache.

4 files changed, 131 insertions, 35 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index caffc6c2bb..0f5faced2e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -8,6 +8,7 @@
 * [**Xabier de Zuazo**](https://github.com/zuazo):
   Remove the unused StreamingCookbookUploader class (CHEF-4586)
 
+* cookbook synchronizer deletes old files from cookbooks
 * do not clear file cache when override run list is set (CHEF-3684)
 * ruby 1.8.7/1.9.1/1.9.2 support is dropped
 * set no_lazy_load to true (CHEF-4961)
diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
index 3f32c5acf6..6cd6b5cb52 100644
--- a/RELEASE_NOTES.md
+++ b/RELEASE_NOTES.md
@@ -1,5 +1,10 @@
 # Chef Client Release Notes:
 
+## Cookbook Synchronizer Cleans Deleted Files
+
+At the start of the Chef client run any files which are in active cookbooks, but are no longer in the
+manifest for the cookbook will be deleted from the cookbook file cache.
+
 ## When given an override run list Chef does not clean the file_cache
 
 In order to avoid redownloading the file_cache for all the cookbooks and files that are skipped when an
diff --git a/lib/chef/cookbook/synchronizer.rb b/lib/chef/cookbook/synchronizer.rb
index 6a986cb141..1b96d0510b 100644
--- a/lib/chef/cookbook/synchronizer.rb
+++ b/lib/chef/cookbook/synchronizer.rb
@@ -45,7 +45,7 @@ class Chef
       unless Chef::Config[:solo] || skip_removal
         # Delete each file in the cache that we didn't encounter in the
         # manifest.
-        cache.find(File.join(%w{cookbooks ** *})).each do |cache_filename|
+        cache.find(File.join(%w{cookbooks ** {*,.*}})).each do |cache_filename|
           unless @valid_cache_entries[cache_filename]
             Chef::Log.info("Removing #{cache_filename} from the cache; it is no longer needed by chef-client.")
             cache.delete(cache_filename)
@@ -99,6 +99,10 @@ class Chef
       @cookbooks_by_name.key?(cookbook_name)
     end
 
+    def cookbook_segment(cookbook_name, segment)
+      @cookbooks_by_name[cookbook_name].manifest[segment]
+    end
+
     def files
       @files ||= cookbooks.inject([]) do |memo, cookbook|
         @eager_segments.each do |segment|
@@ -176,18 +180,10 @@ class Chef
       @cookbook_full_file_paths[file.cookbook][file.segment] << full_path
     end
 
-    # Iterates over cached cookbooks' files, removing files belonging to
-    # cookbooks that don't appear in +cookbook_hash+
-    def clear_obsoleted_cookbooks
-      unless remove_obsoleted_files
-        Chef::Log.info("Skipping removal of obsoleted cookbooks from the cache")
-        CookbookCacheCleaner.instance.skip_removal = true
-        return
-      end
-
-      @events.cookbook_clean_start
-      # Remove all cookbooks no longer relevant to this node
-      cache.find(File.join(%w{cookbooks ** *})).each do |cache_file|
+    # remove cookbooks that are not referenced in the expanded run_list at all
+    # (if we have an override run_list we may not want to do this)
+    def remove_old_cookbooks
+      cache.find(File.join(%w{cookbooks ** {*,.*}})).each do |cache_file|
         cache_file =~ /^cookbooks\/([^\/]+)\//
         unless have_cookbook?($1)
           Chef::Log.info("Removing #{cache_file} from the cache; its cookbook is no longer needed on this client.")
@@ -195,6 +191,39 @@ class Chef
           @events.removed_cookbook_file(cache_file)
         end
       end
+    end
+
+    # remove deleted files in cookbooks that are being used on the node
+    def remove_deleted_files
+      cache.find(File.join(%w{cookbooks ** {*,.*}})).each do |cache_file|
+        md = cache_file.match(/^cookbooks\/([^\/]+)\/([^\/]+)\/(.*)/)
+        next unless md
+        ( cookbook_name, segment, file ) = md[1..3]
+        if have_cookbook?(cookbook_name)
+          manifest_segment = cookbook_segment(cookbook_name, segment)
+          if manifest_segment.select { |manifest_record| manifest_record["path"] == "#{segment}/#{file}" }.empty?
+            Chef::Log.info("Removing #{cache_file} from the cache; its is no longer in the cookbook manifest.")
+            cache.delete(cache_file)
+            @events.removed_cookbook_file(cache_file)
+          end
+        end
+      end
+    end
+
+    # Iterates over cached cookbooks' files, removing files belonging to
+    # cookbooks that don't appear in +cookbook_hash+
+    def clear_obsoleted_cookbooks
+      @events.cookbook_clean_start
+
+      if remove_obsoleted_files
+        remove_old_cookbooks
+      else
+        Chef::Log.info("Skipping removal of obsoleted cookbooks from the cache")
+        CookbookCacheCleaner.instance.skip_removal = true
+      end
+
+      remove_deleted_files
+
       @events.cookbook_clean_complete
     end
 
diff --git a/spec/unit/cookbook/synchronizer_spec.rb b/spec/unit/cookbook/synchronizer_spec.rb
index 9b028064d5..2b040f3c95 100644
--- a/spec/unit/cookbook/synchronizer_spec.rb
+++ b/spec/unit/cookbook/synchronizer_spec.rb
@@ -11,34 +11,50 @@ describe Chef::CookbookCacheCleaner do
       cleaner
     end
 
-    it "removes all files that belong to unused cookbooks" do
-      # lolwut?
+    let(:file_cache) { double("Chef::FileCache with files from unused cookbooks") }
+
+    let(:unused_template_files) do
+      %w{
+        cookbooks/unused/templates/default/foo.conf.erb
+        cookbooks/unused/tempaltes/default/bar.conf.erb
+      }
+    end
+
+    let(:valid_cached_cb_files) do
+      %w{
+        cookbooks/valid1/recipes/default.rb
+        cookbooks/valid2/recipes/default.rb
+      }
+    end
+
+    before do
+      valid_cached_cb_files.each do |cbf|
+        cleaner.mark_file_as_valid(cbf)
+      end
     end
 
     it "removes all files not validated during the chef run" do
-      file_cache = double("Chef::FileCache with files from unused cookbooks")
-      unused_template_files = %w{cookbooks/unused/templates/default/foo.conf.erb cookbooks/unused/tempaltes/default/bar.conf.erb}
-      valid_cached_cb_files = %w{cookbooks/valid1/recipes/default.rb cookbooks/valid2/recipes/default.rb}
-      cleaner.mark_file_as_valid('cookbooks/valid1/recipes/default.rb')
-      cleaner.mark_file_as_valid('cookbooks/valid2/recipes/default.rb')
-      expect(file_cache).to receive(:find).with(File.join(%w{cookbooks ** *})).and_return(valid_cached_cb_files + unused_template_files)
-      expect(file_cache).to receive(:delete).with('cookbooks/unused/templates/default/foo.conf.erb')
-      expect(file_cache).to receive(:delete).with('cookbooks/unused/tempaltes/default/bar.conf.erb')
+      expect(file_cache).to receive(:find).with(File.join(%w{cookbooks ** {*,.*}})).and_return(valid_cached_cb_files + unused_template_files)
+      unused_template_files.each do |cbf|
+        expect(file_cache).to receive(:delete).with(cbf)
+      end
       cookbook_hash = {"valid1"=> {}, "valid2" => {}}
       allow(cleaner).to receive(:cache).and_return(file_cache)
       cleaner.cleanup_file_cache
     end
 
-    describe "on chef-solo" do
-      before do
-        Chef::Config[:solo] = true
-      end
+    it "does not remove anything when skip_removal is true" do
+      cleaner.skip_removal = true
+      allow(cleaner.cache).to receive(:find).and_return(%w{cookbooks/valid1/recipes/default.rb cookbooks/valid2/recipes/default.rb})
+      expect(cleaner.cache).not_to receive(:delete)
+      cleaner.cleanup_file_cache
+    end
 
-      it "does not remove anything" do
-        allow(cleaner.cache).to receive(:find).and_return(%w{cookbooks/valid1/recipes/default.rb cookbooks/valid2/recipes/default.rb})
-        expect(cleaner.cache).not_to receive(:delete)
-        cleaner.cleanup_file_cache
-      end
+    it "does not remove anything on chef-solo" do
+      Chef::Config[:solo] = true
+      allow(cleaner.cache).to receive(:find).and_return(%w{cookbooks/valid1/recipes/default.rb cookbooks/valid2/recipes/default.rb})
+      expect(cleaner.cache).not_to receive(:delete)
+      cleaner.cleanup_file_cache
     end
   end
 end
@@ -115,7 +131,30 @@ describe Chef::CookbookSynchronizer do
     expect(synchronizer.cookbooks).to eq([cookbook_a])
   end
 
-  context "when the cache contains unneeded cookbooks" do
+  context "#clear_obsoleted_cookbooks" do
+    after do
+      # Singletons == Global State == Bad
+      Chef::CookbookCacheCleaner.instance.skip_removal = nil
+    end
+
+    it "behaves correctly when remove_obsoleted_files is false" do
+      synchronizer.remove_obsoleted_files = false
+      expect(synchronizer).not_to receive(:remove_old_cookbooks)
+      expect(synchronizer).to receive(:remove_deleted_files)
+      synchronizer.clear_obsoleted_cookbooks
+      expect(Chef::CookbookCacheCleaner.instance.skip_removal).to be true
+    end
+
+    it "behaves correctly when remove_obsoleted_files is true" do
+      synchronizer.remove_obsoleted_files = true
+      expect(synchronizer).to receive(:remove_old_cookbooks)
+      expect(synchronizer).to receive(:remove_deleted_files)
+      synchronizer.clear_obsoleted_cookbooks
+      expect(Chef::CookbookCacheCleaner.instance.skip_removal).to be nil
+    end
+  end
+
+  context "#remove_old_cookbooks" do
     let(:file_cache) { double("Chef::FileCache with files from unused cookbooks") }
 
     let(:cookbook_manifest) do
@@ -125,11 +164,33 @@ describe Chef::CookbookSynchronizer do
     it "removes unneeded cookbooks" do
       valid_cached_cb_files = %w{cookbooks/valid1/recipes/default.rb cookbooks/valid2/recipes/default.rb}
       obsolete_cb_files = %w{cookbooks/old1/recipes/default.rb cookbooks/old2/recipes/default.rb}
-      expect(file_cache).to receive(:find).with(File.join(%w{cookbooks ** *})).and_return(valid_cached_cb_files + obsolete_cb_files)
+      expect(file_cache).to receive(:find).with(File.join(%w{cookbooks ** {*,.*}})).and_return(valid_cached_cb_files + obsolete_cb_files)
       expect(file_cache).to receive(:delete).with('cookbooks/old1/recipes/default.rb')
       expect(file_cache).to receive(:delete).with('cookbooks/old2/recipes/default.rb')
       allow(synchronizer).to receive(:cache).and_return(file_cache)
-      synchronizer.clear_obsoleted_cookbooks
+      synchronizer.remove_old_cookbooks
+    end
+  end
+
+  context "#remove_deleted_files" do
+    let(:file_cache) { double("Chef::FileCache with files from unused cookbooks") }
+
+    let(:cookbook_manifest) do
+      {"valid1"=> {}, "valid2" => {}}
+    end
+
+    it "removes only deleted files" do
+      valid_cached_cb_files = %w{cookbooks/valid1/recipes/default.rb cookbooks/valid2/recipes/default.rb}
+      obsolete_cb_files = %w{cookbooks/valid1/recipes/deleted.rb cookbooks/valid2/recipes/deleted.rb}
+      expect(file_cache).to receive(:find).with(File.join(%w{cookbooks ** {*,.*}})).and_return(valid_cached_cb_files + obsolete_cb_files)
+      # valid1 is a cookbook in our run_list
+      expect(synchronizer).to receive(:have_cookbook?).with("valid1").at_least(:once).and_return(true)
+      # valid2 is a cookbook not in our run_list (we're simulating an override run_list where valid2 needs to be preserved)
+      expect(synchronizer).to receive(:have_cookbook?).with("valid2").at_least(:once).and_return(false)
+      expect(file_cache).to receive(:delete).with('cookbooks/valid1/recipes/deleted.rb')
+      expect(synchronizer).to receive(:cookbook_segment).with("valid1", "recipes").at_least(:once).and_return([ { "path" => "recipes/default.rb" }])
+      allow(synchronizer).to receive(:cache).and_return(file_cache)
+      synchronizer.remove_deleted_files
     end
   end