Merge pull request #5940 from chef/ssd-sr/re-raise-system-exit

Don't `rescue Exception` in retryable resources

4 files changed, 26 insertions, 9 deletions

diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
index aa8d612d81..5118ce9031 100644
--- a/RELEASE_NOTES.md
+++ b/RELEASE_NOTES.md
@@ -53,6 +53,10 @@ The remediation is removing the self-dependency `depends` line in the metadata.
 
 Retained only for the service resource (where it makes some sense) and for the mount resource.
 
+### Removed retrying of non-StandardError exceptions for Chef::Resource
+
+Exceptions not decending from StandardError (e.g. LoadError, SecurityError, SystemExit) will no longer trigger a retry if they are raised during the executiong of a resources with a non-zero retries setting.
+
 ### Removed deprecated `method_missing` access from the Chef::Node object
 
 Previously, the syntax `node.foo.bar` could be used to mean `node["foo"]["bar"]`, but this API had sharp edges where methods collided
diff --git a/lib/chef/resource.rb b/lib/chef/resource.rb
index b0e3a372e8..f0d816cd89 100644
--- a/lib/chef/resource.rb
+++ b/lib/chef/resource.rb
@@ -589,7 +589,7 @@ class Chef
       begin
         return if should_skip?(action)
         provider_for_action(action).run_action
-      rescue Exception => e
+      rescue StandardError => e
         if ignore_failure
           Chef::Log.error("#{custom_exception_message(e)}; ignore_failure is set, continuing")
           events.resource_failed(self, action, e)
diff --git a/spec/unit/resource_spec.rb b/spec/unit/resource_spec.rb
index a83b22ea0f..a806c5d1d9 100644
--- a/spec/unit/resource_spec.rb
+++ b/spec/unit/resource_spec.rb
@@ -549,6 +549,18 @@ describe Chef::Resource do
       expect { retriable_resource.run_action(:purr) }.to raise_error(RuntimeError)
       expect(retriable_resource.retries).to eq(3)
     end
+
+    it "should not rescue from non-StandardError exceptions" do
+      retriable_resource.retries(3)
+      retriable_resource.retry_delay(0) # No need to wait.
+
+      provider = Chef::Provider::SnakeOil.new(retriable_resource, run_context)
+      allow(Chef::Provider::SnakeOil).to receive(:new).and_return(provider)
+      allow(provider).to receive(:action_purr).and_raise(LoadError)
+
+      expect(retriable_resource).not_to receive(:sleep)
+      expect { retriable_resource.run_action(:purr) }.to raise_error(LoadError)
+    end
   end
 
   it "runs an action by finding its provider, loading the current resource and then running the action" do
diff --git a/spec/unit/runner_spec.rb b/spec/unit/runner_spec.rb
index ea721965c7..eeb471608e 100644
--- a/spec/unit/runner_spec.rb
+++ b/spec/unit/runner_spec.rb
@@ -122,25 +122,26 @@ describe Chef::Runner do
 
     it "should raise exceptions as thrown by a provider" do
       provider = Chef::Provider::SnakeOil.new(run_context.resource_collection[0], run_context)
-      allow(Chef::Provider::SnakeOil).to receive(:new).once.and_return(provider)
-      allow(provider).to receive(:action_sell).once.and_raise(ArgumentError)
+      expect(Chef::Provider::SnakeOil).to receive(:new).once.and_return(provider)
+      expect(provider).to receive(:action_sell).once.and_raise(ArgumentError)
       expect { runner.converge }.to raise_error(ArgumentError)
     end
 
     it "should not raise exceptions thrown by providers if the resource has ignore_failure set to true" do
       allow(run_context.resource_collection[0]).to receive(:ignore_failure).and_return(true)
       provider = Chef::Provider::SnakeOil.new(run_context.resource_collection[0], run_context)
-      allow(Chef::Provider::SnakeOil).to receive(:new).once.and_return(provider)
-      allow(provider).to receive(:action_sell).once.and_raise(ArgumentError)
+      expect(Chef::Provider::SnakeOil).to receive(:new).once.and_return(provider)
+      expect(provider).to receive(:action_sell).once.and_raise(ArgumentError)
       expect { runner.converge }.not_to raise_error
     end
 
     it "should retry with the specified delay if retries are specified" do
-      first_resource.retries 3
+      num_retries = 3
+      allow(run_context.resource_collection[0]).to receive(:retries).and_return(num_retries)
       provider = Chef::Provider::SnakeOil.new(run_context.resource_collection[0], run_context)
-      allow(Chef::Provider::SnakeOil).to receive(:new).once.and_return(provider)
-      allow(provider).to receive(:action_sell).and_raise(ArgumentError)
-      expect(first_resource).to receive(:sleep).with(2).exactly(3).times
+      expect(Chef::Provider::SnakeOil).to receive(:new).exactly(num_retries + 1).times.and_return(provider)
+      expect(provider).to receive(:action_sell).exactly(num_retries + 1).times.and_raise(ArgumentError)
+      expect(run_context.resource_collection[0]).to receive(:sleep).with(2).exactly(num_retries).times
       expect { runner.converge }.to raise_error(ArgumentError)
     end