Don't `rescue Exception` for retriable resources

Without this, sending an interrupt signal (hitting Ctrl+C) during a
chef-client run that is currently executing a resources that has
`retries` set will not interrupt the run, although the interrupt handler
set up in Chef::Application calls Process.exit, which raises SystemExit.

If the retryable resource is shelling out, the spawned process also gets
the INT signal and will stop -- however, the chef-run would go on,
merely noticing that the resource has failed and theres X-1 retries
left.

It is imaginable that users depend on some (whacky) Ruby script in a
retriable resource that raises SystemExit, and they want this to _not_
stop the chef-run. So it's up for debate if we just want to merge this
or make it configurable.

In general, I'd like Ctrl+C to _stop my chef-run_, but this expectation
could also be misled.

When adapting tests to this change, it became apparent that for some
reason the runner_spec.rb test didn't behave as expected. I've thus made
those tests a little stricter by replacing some allows by expects.

Signed-off-by: Stephan Renatus <srenatus@chef.io>

2 files changed, 21 insertions, 8 deletions

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