vmalert: fix the access to rules slice element by wrong index (#486)

During group's update rules deletion was causing slice
mutations while slice index was assumed to be unchanged.
This caused "slice bounds out of range" errors when multiple
rules were deleted sequentially.

app/vmalert/group.go

@@ -47,7 +47,8 @@ func (g *Group) Restore(ctx context.Context, q datasource.Querier, lookback time
 }
 
 // updateWith updates existing group with
-// passed group object.
+// passed group object. Must be called
+// under mutex lock.
 func (g *Group) updateWith(newGroup Group) {
 	rulesRegistry := make(map[string]*Rule)
 	for _, nr := range newGroup.Rules {
@@ -58,9 +59,8 @@ func (g *Group) updateWith(newGroup Group) {
 		nr, ok := rulesRegistry[or.id()]
 		if !ok {
 			// old rule is not present in the new list
-			// and must be removed
-			or = nil
-			g.Rules = append(g.Rules[:i], g.Rules[i+1:]...)
+			// so we mark it for removing
+			g.Rules[i] = nil
 			continue
 		}
 
@@ -74,9 +74,19 @@ func (g *Group) updateWith(newGroup Group) {
 		delete(rulesRegistry, nr.id())
 	}
 
-	for _, nr := range rulesRegistry {
-		g.Rules = append(g.Rules, nr)
+	var newRules []*Rule
+	for _, r := range g.Rules {
+		if r == nil {
+			// skip nil rules
+			continue
+		}
+		newRules = append(newRules, r)
 	}
+	// add the rest of rules from registry
+	for _, nr := range rulesRegistry {
+		newRules = append(newRules, nr)
+	}
+	g.Rules = newRules
 }
 
 var (

app/vmalert/group_test.go

@@ -15,7 +15,8 @@ func TestUpdateWith(t *testing.T) {
 	testCases := []struct {
 		name         string
 		currentRules []*Rule
-		newRules     []*Rule
+		// rules must be sorted by name
+		newRules []*Rule
 	}{
 		{
 			"new rule",
@@ -55,8 +56,18 @@ func TestUpdateWith(t *testing.T) {
 		},
 		{
 			"multiple rules",
-			[]*Rule{{Name: "foo"}, {Name: "bar"}, {Name: "baz"}},
-			[]*Rule{{Name: "foo"}, {Name: "baz"}},
+			[]*Rule{{Name: "bar"}, {Name: "baz"}, {Name: "foo"}},
+			[]*Rule{{Name: "baz"}, {Name: "foo"}},
+		},
+		{
+			"replace rule",
+			[]*Rule{{Name: "foo1"}},
+			[]*Rule{{Name: "foo2"}},
+		},
+		{
+			"replace multiple rules",
+			[]*Rule{{Name: "foo1"}, {Name: "foo2"}},
+			[]*Rule{{Name: "foo3"}, {Name: "foo4"}},
 		},
 	}
 
@@ -69,6 +80,9 @@ func TestUpdateWith(t *testing.T) {
 				t.Fatalf("expected to have %d rules; got: %d",
 					len(g.Rules), len(tc.newRules))
 			}
+			sort.Slice(g.Rules, func(i, j int) bool {
+				return g.Rules[i].Name < g.Rules[j].Name
+			})
 			for i, r := range g.Rules {
 				got, want := r, tc.newRules[i]
 				if got.Name != want.Name {