Fix patch4 transition

This deals with some more cases that may happen when going from
patch level 3 to 4:
- skip change that are already done and not migrate them.
- do not assume that "had-candidate" is set, it may e.g. not be
  set yet (because the task has not run yet)

overlord/patch/patch4.go

@@ -206,7 +206,7 @@ func (p4 patch4T) mangle(task *state.Task) error {
 	}
 
 	var hadCandidate bool
-	if err := p4.get(task, "had-candidate", &hadCandidate); err != nil {
+	if err := p4.getMaybe(task, "had-candidate", &hadCandidate); err != nil && err != state.ErrNoState {
 		return err
 	}
 
@@ -245,6 +245,11 @@ func (p4 patch4T) addRevertFlag(task *state.Task) error {
 func patch4(s *state.State) error {
 	p4 := patch4T{}
 	for _, change := range s.Changes() {
+		// change is full done, take it easy
+		if change.Status().Ready() {
+			continue
+		}
+
 		if change.Kind() != "revert-snap" {
 			continue
 		}
@@ -256,6 +261,11 @@ func patch4(s *state.State) error {
 	}
 
 	for _, task := range s.Tasks() {
+		// change is full done, take it easy
+		if task.Change().Status().Ready() {
+			continue
+		}
+
 		switch task.Kind() {
 		case "link-snap":
 			if err := p4.mangle(task); err != nil {

overlord/patch/patch4_test.go

@@ -62,15 +62,15 @@ var statePatch4JSON = []byte(`
 			"id": "1",
 			"kind": "revert-snap",
 			"summary": "revert a snap",
-			"status": 0,
+			"status": 2,
 			"data": {"snap-names": ["a"]},
 			"task-ids": ["1","2","3","4"]
 		},
 		"2": {
 			"id": "2",
 			"kind": "refresh-snap",
 			"summary": "refresh b snap",
-			"status": 0,
+			"status": 2,
 			"data": {"snap-names": ["b"]},
 			"task-ids": ["10","11","12","13","14","15","16"]
 		}
@@ -215,8 +215,11 @@ func (s *patch4Suite) TestPatch4OnReverts(c *C) {
 		st.Lock()
 		defer st.Unlock()
 
+		// simulate that the task was running (but the change
+		// is not fully done yet)
 		task := st.Task("4")
 		c.Assert(task, NotNil)
+		task.SetStatus(state.DoneStatus)
 
 		ss, err := snapstate.TaskSnapSetup(task)
 		c.Assert(err, IsNil)
@@ -252,6 +255,59 @@ func (s *patch4Suite) TestPatch4OnReverts(c *C) {
 	c.Check(len(task.Change().Tasks()), Equals, 4)
 }
 
+func (s *patch4Suite) TestPatch4OnRevertsNoCandidateYet(c *C) {
+	restorer := patch.MockLevel(4)
+	defer restorer()
+
+	r, err := os.Open(dirs.SnapStateFile)
+	c.Assert(err, IsNil)
+	defer r.Close()
+	st, err := state.ReadState(nil, r)
+	c.Assert(err, IsNil)
+
+	func() {
+		st.Lock()
+		defer st.Unlock()
+
+		task := st.Task("4")
+		c.Assert(task, NotNil)
+		// its ready to run but has not run yet
+		task.Clear("had-candidate")
+		task.SetStatus(state.DoStatus)
+
+		ss, err := snapstate.TaskSnapSetup(task)
+		c.Assert(err, IsNil)
+		c.Check(ss.Flags.Revert(), Equals, false)
+
+		var had bool
+		var idx int
+		c.Check(task.Get("had-candidate", &had), Equals, state.ErrNoState)
+		c.Check(task.Get("old-candidate-index", &idx), Equals, state.ErrNoState)
+		c.Check(len(task.Change().Tasks()), Equals, 4)
+	}()
+
+	// go from patch level 3 -> 4
+	err = patch.Apply(st)
+	c.Assert(err, IsNil)
+
+	st.Lock()
+	defer st.Unlock()
+
+	task := st.Task("4")
+	c.Assert(task, NotNil)
+
+	ss, err := snapstate.TaskSnapSetup(task)
+	c.Assert(err, IsNil)
+	c.Check(ss.Flags.Revert(), Equals, true)
+
+	var had bool
+	var idx int
+	c.Check(task.Get("had-candidate", &had), Equals, state.ErrNoState)
+	c.Check(task.Get("old-candidate-index", &idx), IsNil)
+	c.Check(idx, Equals, 1)
+	c.Check(len(task.Change().Tasks()), Equals, 4)
+}
+
 func (s *patch4Suite) TestPatch4OnRefreshes(c *C) {
 	restorer := patch.MockLevel(4)
 	defer restorer()
@@ -268,6 +324,9 @@ func (s *patch4Suite) TestPatch4OnRefreshes(c *C) {
 
 		task := st.Task("16")
 		c.Assert(task, NotNil)
+		// simulate that the task was running (but the change
+		// is not fully done yet)
+		task.SetStatus(state.DoneStatus)
 
 		ss, err := snapstate.TaskSnapSetup(task)
 		c.Assert(err, IsNil)
@@ -303,3 +362,59 @@ func (s *patch4Suite) TestPatch4OnRefreshes(c *C) {
 	// we added cleanup
 	c.Check(len(task.Change().Tasks()), Equals, 7+1)
 }
+
+// This test simulates a link-snap task that is scheduled but has not
+// run yet. It has no "had-candidate" data set yet.
+func (s *patch4Suite) TestPatch4OnRefreshesNoHadCandidateYet(c *C) {
+	restorer := patch.MockLevel(4)
+	defer restorer()
+
+	r, err := os.Open(dirs.SnapStateFile)
+	c.Assert(err, IsNil)
+	defer r.Close()
+	st, err := state.ReadState(nil, r)
+	c.Assert(err, IsNil)
+
+	func() {
+		st.Lock()
+		defer st.Unlock()
+
+		task := st.Task("16")
+		c.Assert(task, NotNil)
+		// its ready to run but has not run yet
+		task.Clear("had-candidate")
+		task.SetStatus(state.DoStatus)
+
+		ss, err := snapstate.TaskSnapSetup(task)
+		c.Assert(err, IsNil)
+		c.Check(ss.Flags.Revert(), Equals, false)
+
+		var had bool
+		var idx int
+		c.Check(task.Get("had-candidate", &had), Equals, state.ErrNoState)
+		c.Check(task.Get("old-candidate-index", &idx), Equals, state.ErrNoState)
+		c.Check(len(task.Change().Tasks()), Equals, 7)
+	}()
+
+	// go from patch level 3 -> 4
+	err = patch.Apply(st)
+	c.Assert(err, IsNil)
+
+	st.Lock()
+	defer st.Unlock()
+
+	task := st.Task("16")
+	c.Assert(task, NotNil)
+
+	ss, err := snapstate.TaskSnapSetup(task)
+	c.Assert(err, IsNil)
+	c.Check(ss.Flags.Revert(), Equals, false)
+
+	var had bool
+	var idx int
+	c.Check(task.Get("had-candidate", &had), Equals, state.ErrNoState)
+	c.Check(task.Get("old-candidate-index", &idx), IsNil)
+	c.Check(idx, Equals, 1)
+	// we added cleanup
+	c.Check(len(task.Change().Tasks()), Equals, 7+1)
+}