Merge pull request #30958 from hashicorp/jbardin/dependencies-race

Fix race in state dependency encoding

Author: jbardin

.github/workflows/checks.yml

@@ -96,7 +96,7 @@ jobs:
       # it for select packages.
       - name: "Race detector"
         run: |
-          go test -race ./internal/terraform ./internal/command
+          go test -race ./internal/terraform ./internal/command ./internal/states
 
   e2e-tests:
     # This is an intentionally-limited form of our E2E test run which only

internal/states/instance_object.go

@@ -115,15 +115,20 @@ func (o *ResourceInstanceObject) Encode(ty cty.Type, schemaVersion uint64) (*Res
 	// stored in state as an array. To avoid pointless thrashing of state in
 	// refresh-only runs, we can either override comparison of dependency lists
 	// (more desirable, but tricky for Reasons) or just sort when encoding.
-	sort.Slice(o.Dependencies, func(i, j int) bool { return o.Dependencies[i].String() < o.Dependencies[j].String() })
+	// Encoding of instances can happen concurrently, so we must copy the
+	// dependencies to avoid mutating what may be a shared array of values.
+	dependencies := make([]addrs.ConfigResource, len(o.Dependencies))
+	copy(dependencies, o.Dependencies)
+
+	sort.Slice(dependencies, func(i, j int) bool { return dependencies[i].String() < dependencies[j].String() })
 
 	return &ResourceInstanceObjectSrc{
 		SchemaVersion:       schemaVersion,
 		AttrsJSON:           src,
 		AttrSensitivePaths:  pvm,
 		Private:             o.Private,
 		Status:              o.Status,
-		Dependencies:        o.Dependencies,
+		Dependencies:        dependencies,
 		CreateBeforeDestroy: o.CreateBeforeDestroy,
 	}, nil
 }

internal/states/instance_object_test.go

@@ -1,6 +1,7 @@
 package states
 
 import (
+	"sync"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -24,27 +25,59 @@ func TestResourceInstanceObject_encode(t *testing.T) {
 		addrs.RootModule.Resource(addrs.ManagedResourceMode, "test", "boop"),
 		addrs.RootModule.Resource(addrs.ManagedResourceMode, "test", "honk"),
 	}
-	rioOne := &ResourceInstanceObject{
-		Value:        value,
-		Status:       ObjectPlanned,
-		Dependencies: depsOne,
-	}
-	rioTwo := &ResourceInstanceObject{
-		Value:        value,
-		Status:       ObjectPlanned,
-		Dependencies: depsTwo,
-	}
-	riosOne, err := rioOne.Encode(value.Type(), 0)
-	if err != nil {
-		t.Fatalf("unexpected error: %s", err)
+
+	// multiple instances may have been assigned the same deps slice
+	objs := []*ResourceInstanceObject{
+		&ResourceInstanceObject{
+			Value:        value,
+			Status:       ObjectPlanned,
+			Dependencies: depsOne,
+		},
+		&ResourceInstanceObject{
+			Value:        value,
+			Status:       ObjectPlanned,
+			Dependencies: depsTwo,
+		},
+		&ResourceInstanceObject{
+			Value:        value,
+			Status:       ObjectPlanned,
+			Dependencies: depsOne,
+		},
+		&ResourceInstanceObject{
+			Value:        value,
+			Status:       ObjectPlanned,
+			Dependencies: depsOne,
+		},
 	}
-	riosTwo, err := rioTwo.Encode(value.Type(), 0)
-	if err != nil {
-		t.Fatalf("unexpected error: %s", err)
+
+	var encoded []*ResourceInstanceObjectSrc
+
+	// Encoding can happen concurrently, so we need to make sure the shared
+	// Dependencies are safely handled
+	var wg sync.WaitGroup
+	var mu sync.Mutex
+
+	for _, obj := range objs {
+		obj := obj
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			rios, err := obj.Encode(value.Type(), 0)
+			if err != nil {
+				t.Errorf("unexpected error: %s", err)
+			}
+			mu.Lock()
+			encoded = append(encoded, rios)
+			mu.Unlock()
+		}()
 	}
+	wg.Wait()
+
 	// However, identical sets of dependencies should always be written to state
 	// in an identical order, so we don't do meaningless state updates on refresh.
-	if diff := cmp.Diff(riosOne.Dependencies, riosTwo.Dependencies); diff != "" {
-		t.Errorf("identical dependencies got encoded in different orders:\n%s", diff)
+	for i := 0; i < len(encoded)-1; i++ {
+		if diff := cmp.Diff(encoded[i].Dependencies, encoded[i+1].Dependencies); diff != "" {
+			t.Errorf("identical dependencies got encoded in different orders:\n%s", diff)
+		}
 	}
 }