Fix bug lp#1694734

worker/uniter/actions/resolver.go

@@ -42,13 +42,13 @@ func (r *actionsResolver) NextOp(
 	opFactory operation.Factory,
 ) (operation.Operation, error) {
 	nextAction, err := nextAction(remoteState.Actions, localState.CompletedActions)
-	if err != nil {
+	if err != nil && err != resolver.ErrNoOperation {
 		return nil, err
 	}
 	switch localState.Kind {
 	case operation.RunHook:
 		// We can still run actions if the unit is in a hook error state.
-		if localState.Step == operation.Pending {
+		if localState.Step == operation.Pending && err != resolver.ErrNoOperation {
 			return opFactory.NewAction(nextAction)
 		}
 	case operation.RunAction:
@@ -58,10 +58,31 @@ func (r *actionsResolver) NextOp(
 			return opFactory.NewSkipHook(*localState.Hook)
 		} else {
 			logger.Infof("%q hook is nil", operation.RunAction)
-			return opFactory.NewFailAction(*localState.ActionId)
+
+			// If the next action is the same as what the uniter is
+			// currently running then this means that the uniter was
+			// some how interrupted (killed) when running the action
+			// and before updating the remote state to indicate that
+			// the action was completed. The only safe thing to do
+			// is fail the action, since rerunning an arbitrary
+			// command can potentially be hazardous.
+			if nextAction == *localState.ActionId {
+				return opFactory.NewFailAction(*localState.ActionId)
+			}
+
+			// If the next action is different then what the uniter
+			// is currently running, then the uniter may have been
+			// interrupted while running the action but the remote
+			// state was updated. Thus, the semantics of
+			// (re)preparing the running operation should move the
+			// uniter's state along safely. Thus, we return the
+			// running action.
+			return opFactory.NewAction(*localState.ActionId)
 		}
 	case operation.Continue:
-		return opFactory.NewAction(nextAction)
+		if err != resolver.ErrNoOperation {
+			return opFactory.NewAction(nextAction)
+		}
 	}
 	return nil, resolver.ErrNoOperation
 }

worker/uniter/actions/resolver_test.go

@@ -72,6 +72,44 @@ func (s *actionsSuite) TestNextAction(c *gc.C) {
 	c.Assert(op, jc.DeepEquals, mockOp("actionB"))
 }
 
+func (s *actionsSuite) TestActionStateKindRunAction(c *gc.C) {
+	actionResolver := actions.NewResolver()
+	var actionA string = "actionA"
+
+	localState := resolver.LocalState{
+		State: operation.State{
+			Kind:     operation.RunAction,
+			ActionId: &actionA,
+		},
+		CompletedActions: map[string]struct{}{},
+	}
+	remoteState := remotestate.Snapshot{
+		Actions: []string{},
+	}
+	op, err := actionResolver.NextOp(localState, remoteState, &mockOperations{})
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(op, jc.DeepEquals, mockOp("actionA"))
+}
+
+func (s *actionsSuite) TestActionStateKindRunActionPendingRemote(c *gc.C) {
+	actionResolver := actions.NewResolver()
+	var actionA string = "actionA"
+
+	localState := resolver.LocalState{
+		State: operation.State{
+			Kind:     operation.RunAction,
+			ActionId: &actionA,
+		},
+		CompletedActions: map[string]struct{}{},
+	}
+	remoteState := remotestate.Snapshot{
+		Actions: []string{"actionA", "actionB"},
+	}
+	op, err := actionResolver.NextOp(localState, remoteState, &mockOperations{})
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(op, jc.DeepEquals, mockFailAction("actionA"))
+}
+
 type mockOperations struct {
 	operation.Factory
 }
@@ -80,10 +118,18 @@ func (m *mockOperations) NewAction(id string) (operation.Operation, error) {
 	return mockOp(id), nil
 }
 
+func (m *mockOperations) NewFailAction(id string) (operation.Operation, error) {
+	return mockFailAction(id), nil
+}
+
 func mockOp(name string) operation.Operation {
 	return &mockOperation{name: name}
 }
 
+func mockFailAction(name string) operation.Operation {
+	return &mockFailOp{name: name}
+}
+
 type mockOperation struct {
 	operation.Operation
 	name string
@@ -92,3 +138,12 @@ type mockOperation struct {
 func (op *mockOperation) String() string {
 	return op.name
 }
+
+type mockFailOp struct {
+	operation.Operation
+	name string
+}
+
+func (op *mockFailOp) String() string {
+	return op.name
+}