From 50d7b0aba44a4f3234cdfe3fb4f105f4e082261e Mon Sep 17 00:00:00 2001
From: Mitchell Hashimoto <mitchell.hashimoto@gmail.com>
Date: Sat, 22 Dec 2012 22:16:17 -0800
Subject: [PATCH] Fix bug where Call didn't propagate recovery. Warden has no
 recovery.

The issue here is that when a middleware failed and a recovery sequence
started, it would halt at the "call" step because the "Call" didn't
properly recover the child sequence.

An additional issue was that a Warden had no "recover" method, meaning
embedded Wardens wouldn't recover their stacks properly.
---
 lib/vagrant/action/builder.rb                 | 20 +++----
 lib/vagrant/action/builtin/call.rb            |  9 ++-
 lib/vagrant/action/warden.rb                  | 14 +++--
 test/unit/vagrant/action/builtin/call_test.rb | 59 +++++++++++++++++++
 4 files changed, 86 insertions(+), 16 deletions(-)

diff --git a/lib/vagrant/action/builder.rb b/lib/vagrant/action/builder.rb
index a13912403..23d099a5d 100644
--- a/lib/vagrant/action/builder.rb
+++ b/lib/vagrant/action/builder.rb
@@ -104,16 +104,6 @@ module Vagrant
         nil
       end
 
-      protected
-
-      # Returns the current stack of middlewares. You probably won't
-      # need to use this directly, and it's recommended that you don't.
-      #
-      # @return [Array]
-      def stack
-        @stack ||= []
-      end
-
       # Converts the builder stack to a runnable action sequence.
       #
       # @param [Vagrant::Action::Environment] env The action environment
@@ -123,6 +113,16 @@ module Vagrant
         # and predictable behavior upon exceptions.
         Warden.new(stack.dup, env)
       end
+
+      protected
+
+      # Returns the current stack of middlewares. You probably won't
+      # need to use this directly, and it's recommended that you don't.
+      #
+      # @return [Array]
+      def stack
+        @stack ||= []
+      end
     end
   end
 end
diff --git a/lib/vagrant/action/builtin/call.rb b/lib/vagrant/action/builtin/call.rb
index 2d29a68de..34cf59f30 100644
--- a/lib/vagrant/action/builtin/call.rb
+++ b/lib/vagrant/action/builtin/call.rb
@@ -29,6 +29,7 @@ module Vagrant
           @app      = app
           @callable = callable
           @block    = block
+          @child_app = nil
         end
 
         def call(env)
@@ -42,11 +43,17 @@ module Vagrant
           @block.call(new_env, builder)
 
           # Run the result with our new environment
-          final_env = runner.run(builder, new_env)
+          @child_app = builder.to_app(new_env)
+          final_env = runner.run(@child_app, new_env)
 
           # Call the next step using our final environment
           @app.call(final_env)
         end
+
+        def recover(env)
+          # Call back into our compiled application and recover it.
+          @child_app.recover(env) if @child_app
+        end
       end
     end
   end
diff --git a/lib/vagrant/action/warden.rb b/lib/vagrant/action/warden.rb
index edd898b09..5629a13f2 100644
--- a/lib/vagrant/action/warden.rb
+++ b/lib/vagrant/action/warden.rb
@@ -49,15 +49,17 @@ module Vagrant
 
           # Something went horribly wrong. Start the rescue chain then
           # reraise the exception to properly kick us out of limbo here.
-          begin_rescue(env)
+          recover(env)
           raise
         end
       end
 
-      # Begins the recovery sequence for all middlewares which have run.
-      # It does this by calling `recover` (if it exists) on each middleware
-      # which has already run, in reverse order.
-      def begin_rescue(env)
+      # We implement the recover method ourselves in case a Warden is
+      # embedded within another Warden. To recover, we just do our own
+      # recovery process on our stack.
+      def recover(env)
+        @logger.info("Beginning recovery process...")
+
         @stack.each do |act|
           if act.respond_to?(:recover)
             @logger.info("Calling recover: #{act}")
@@ -65,6 +67,8 @@ module Vagrant
           end
         end
 
+        @logger.info("Recovery complete.")
+
         # Clear stack so that warden down the middleware chain doesn't
         # rescue again.
         @stack.clear
diff --git a/test/unit/vagrant/action/builtin/call_test.rb b/test/unit/vagrant/action/builtin/call_test.rb
index c2ef76bdc..58eb4969b 100644
--- a/test/unit/vagrant/action/builtin/call_test.rb
+++ b/test/unit/vagrant/action/builtin/call_test.rb
@@ -52,4 +52,63 @@ describe Vagrant::Action::Builtin::Call do
 
     received.should == :bar
   end
+
+  it "should call the recover method for the sequence in an error" do
+    # Basic variables
+    callable = lambda { |env| }
+
+    # Build the steps for the test
+    basic_step = Class.new do
+      def initialize(app, env)
+        @app = app
+        @env = env
+      end
+
+      def call(env)
+        @app.call(env)
+      end
+    end
+
+    step_a = Class.new(basic_step) do
+      def call(env)
+        env[:steps] << :call_A
+        super
+      end
+
+      def recover(env)
+        env[:steps] << :recover_A
+      end
+    end
+
+    step_b = Class.new(basic_step) do
+      def call(env)
+        env[:steps] << :call_B
+        super
+      end
+
+      def recover(env)
+        env[:steps] << :recover_B
+      end
+    end
+
+    instance = described_class.new(app, env, callable) do |_env, builder|
+      builder.use step_a
+      builder.use step_b
+    end
+
+    env[:steps] = []
+    instance.call(env)
+    instance.recover(env)
+
+    env[:steps].should == [:call_A, :call_B, :recover_B, :recover_A]
+  end
+
+  it "should recover even if it failed in the callable" do
+    callable = lambda { |env| raise "error" }
+
+    instance = described_class.new(app, env, callable) { |_env, _builder| }
+    instance.call(env) rescue nil
+    expect { instance.recover(env) }.
+      to_not raise_error
+  end
 end