Revert "Run integration tests and dev builds with race detection" (#1…

…5998)

<!--- 
Thanks so much for your contribution! If this is your first time
contributing, please ensure that you have read the
[CONTRIBUTING](https://github.com/pulumi/pulumi/blob/master/CONTRIBUTING.md)
documentation.
-->

# Description

<!--- Please include a summary of the change and which issue is fixed.
Please also include relevant motivation and context. -->

This reverts the sdk and pkg changes from commit
655b76d.

It also disables race detection from all builds and tests.

Fixes #15991.

## Checklist

- [x] I have run `make tidy` to update any new dependencies
- [x] I have run `make lint` to verify my code passes the lint check
  - [ ] I have formatted my code using `gofumpt`

<!--- Please provide details if the checkbox below is to be left
unchecked. -->
- [ ] I have added tests that prove my fix is effective or that my
feature works
<!--- 
User-facing changes require a CHANGELOG entry.
-->
- [x] I have run `make changelog` and committed the
`changelog/pending/<file>` documenting my change
<!--
If the change(s) in this PR is a modification of an existing call to the
Pulumi Cloud,
then the service should honor older versions of the CLI where this
change would not exist.
You must then bump the API version in
/pkg/backend/httpstate/client/api.go, as well as add
it to the service.
-->
- [ ] Yes, there are changes in this PR that warrants bumping the Pulumi
Cloud API version
<!-- @pulumi employees: If yes, you must submit corresponding changes in
the service repo. -->

.github/workflows/ci.yml

@@ -287,8 +287,6 @@ jobs:
       build-platform: ${{ matrix.target.build-platform }}
       version-set: ${{ needs.matrix.outputs.version-set }}
       enable-coverage: ${{ inputs.enable-coverage }}
-      # Windows and Linux need CGO and cross compile support to support -race. So for now only enable it for darwin and amd64 linux.
-      enable-race-detection: ${{ matrix.target.os == 'darwin' || (matrix.target.os == 'linux' && matrix.target.arch == 'amd64') }}
     secrets: inherit
 
   build-sdks:

build/common.mk

@@ -109,7 +109,7 @@ GO_TEST_PARALLELISM     ?= 10   # -parallel, number of parallel tests to run wit
 GO_TEST_PKG_PARALLELISM ?= 2    # -p flag, number of parallel packages to test
 GO_TEST_SHUFFLE         ?= off  # -shuffle flag, randomizes order of tests within a package
 GO_TEST_TAGS            ?= all
-GO_TEST_RACE            ?= true
+GO_TEST_RACE            ?= false # disable race detector by default until snapshot system doesn't race with the engine
 
 GO_TEST_FLAGS = -count=1 -cover -tags="${GO_TEST_TAGS}" -timeout 1h \
 	-parallel=${GO_TEST_PARALLELISM} \

changelog/pending/20240419--engine--remove-locks-between-snapshot-and-executor-systems.yaml

@@ -0,0 +1,4 @@
+changes:
+- type: fix
+  scope: engine
+  description: Remove locks between snapshot and executor systems

pkg/backend/inmemoryPersister.go

@@ -33,19 +33,13 @@ func (p *InMemoryPersister) Save(snap *deploy.Snapshot) error {
 		PendingOperations: make([]resource.Operation, len(snap.PendingOperations)),
 	}
 
-	for i, res := range snap.Resources {
-		res.Lock.Lock()
-		result.Resources[i] = res.Copy()
-		res.Lock.Unlock()
-	}
+	copy(result.Resources, snap.Resources)
 
 	for i, op := range snap.PendingOperations {
-		op.Resource.Lock.Lock()
 		result.PendingOperations[i] = resource.Operation{
 			Type:     op.Type,
-			Resource: op.Resource.Copy(),
+			Resource: op.Resource,
 		}
-		op.Resource.Lock.Unlock()
 	}
 
 	p.Snap = result

pkg/cmd/pulumi/import.go

@@ -500,9 +500,9 @@ func generateImportedDefinitions(ctx *plugin.Context,
 		urn := resource.NewURN(stackName.Q(), projectName, parentType, i.Type, i.Name)
 		if state, ok := resourceTable[urn]; ok {
 			// Copy the state and override the protect bit.
-			s := state.Copy()
+			s := *state
 			s.Protect = protectResources
-			resources = append(resources, s)
+			resources = append(resources, &s)
 		}
 	}
 

pkg/engine/lifecycletest/refresh_test.go

@@ -596,9 +596,6 @@ func validateRefreshBasicsCombination(t *testing.T, names []string, targets []st
 						assert.Equal(t, deploy.OpUpdate, resultOp)
 					}
 
-					old = old.Copy()
-					new = new.Copy()
-
 					// Only the inputs and outputs should have changed (if anything changed).
 					old.Inputs = expected.Inputs
 					old.Outputs = expected.Outputs
@@ -770,7 +767,6 @@ func TestCanceledRefresh(t *testing.T) {
 				}
 
 				// Only the outputs and Modified timestamp should have changed (if anything changed).
-				old = old.Copy()
 				old.Outputs = expected
 				old.Modified = new.Modified
 

pkg/resource/deploy/import.go

@@ -152,11 +152,11 @@ func (i *importer) registerExistingResources(ctx context.Context) bool {
 			}
 
 			// Clear the ID because Same asserts that the new state has no ID.
-			new := r.Copy()
+			new := *r
 			new.ID = ""
 			// Set a dummy goal so the resource is tracked as managed.
 			i.deployment.goals.Store(r.URN, &resource.Goal{})
-			if !i.executeSerial(ctx, NewSameStep(i.deployment, noopEvent(0), r, new)) {
+			if !i.executeSerial(ctx, NewSameStep(i.deployment, noopEvent(0), r, &new)) {
 				return false
 			}
 		}
@@ -348,11 +348,11 @@ func (i *importer) importResources(ctx context.Context) error {
 			}
 			if oldID == imp.ID {
 				// Clear the ID because Same asserts that the new state has no ID.
-				new := old.Copy()
+				new := *old
 				new.ID = ""
 				// Set a dummy goal so the resource is tracked as managed.
 				i.deployment.goals.Store(old.URN, &resource.Goal{})
-				steps = append(steps, NewSameStep(i.deployment, noopEvent(0), old, new))
+				steps = append(steps, NewSameStep(i.deployment, noopEvent(0), old, &new))
 				continue
 			}
 		}
@@ -366,11 +366,11 @@ func (i *importer) importResources(ctx context.Context) error {
 			}
 			if oldID == imp.ID {
 				// Clear the ID because Same asserts that the new state has no ID.
-				new := old.Copy()
+				new := *old
 				new.ID = ""
 				// Set a dummy goal so the resource is tracked as managed.
 				i.deployment.goals.Store(old.URN, &resource.Goal{})
-				steps = append(steps, NewSameStep(i.deployment, noopEvent(0), old, new))
+				steps = append(steps, NewSameStep(i.deployment, noopEvent(0), old, &new))
 				continue
 			}
 		}

pkg/resource/deploy/snapshot.go

@@ -103,9 +103,6 @@ func (snap *Snapshot) NormalizeURNReferences() (*Snapshot, error) {
 	}
 
 	fixResource := func(old *resource.State) *resource.State {
-		old.Lock.Lock()
-		defer old.Lock.Unlock()
-
 		return newStateBuilder(old).
 			withUpdatedURN(fixUrn).
 			withUpdatedParent(fixUrn).

pkg/resource/deploy/snapshot_test.go

@@ -56,9 +56,9 @@ func TestSnapshotWithUpdatedResources(t *testing.T) {
 	assert.Same(t, s, s1)
 
 	s = s1.withUpdatedResources(func(r *resource.State) *resource.State {
-		out := r.Copy()
+		out := *r
 		out.URN += "!"
-		return out
+		return &out
 	})
 	assert.NotSame(t, s, s1)
 	assert.Equal(t, s1.Resources[0].URN+"!", s.Resources[0].URN)

pkg/resource/deploy/state_builder.go

@@ -26,7 +26,7 @@ type stateBuilder struct {
 }
 
 func newStateBuilder(state *resource.State) *stateBuilder {
-	return &stateBuilder{*(state.Copy()), state, false}
+	return &stateBuilder{*state, state, false}
 }
 
 func (sb *stateBuilder) withUpdatedURN(update func(resource.URN) resource.URN) *stateBuilder {

pkg/resource/deploy/step.go

@@ -107,11 +107,11 @@ func NewSkippedCreateStep(deployment *Deployment, reg RegisterResourceEvent, new
 	contract.Requiref(!new.Delete, "new", "must not be marked for deletion")
 
 	// Make the old state here a direct copy of the new state
-	old := new.Copy()
+	old := *new
 	return &SameStep{
 		deployment:    deployment,
 		reg:           reg,
-		old:           old,
+		old:           &old,
 		new:           new,
 		skippedCreate: true,
 	}
@@ -128,9 +128,6 @@ func (s *SameStep) Res() *resource.State    { return s.new }
 func (s *SameStep) Logical() bool           { return true }
 
 func (s *SameStep) Apply(preview bool) (resource.Status, StepCompleteFunc, error) {
-	s.new.Lock.Lock()
-	defer s.new.Lock.Unlock()
-
 	// Retain the ID and outputs
 	s.new.ID = s.old.ID
 	s.new.Outputs = s.old.Outputs
@@ -272,9 +269,6 @@ func (s *CreateStep) Apply(preview bool) (resource.Status, StepCompleteFunc, err
 		}
 	}
 
-	s.new.Lock.Lock()
-	defer s.new.Lock.Unlock()
-
 	// Copy any of the default and output properties on the live object state.
 	s.new.ID = id
 	s.new.Outputs = outs
@@ -287,9 +281,7 @@ func (s *CreateStep) Apply(preview bool) (resource.Status, StepCompleteFunc, err
 	// Mark the old resource as pending deletion if necessary.
 	if s.replacing && s.pendingDelete {
 		contract.Assertf(s.old != s.new, "old and new states should not be the same")
-		s.old.Lock.Lock()
 		s.old.Delete = true
-		s.old.Lock.Unlock()
 	}
 
 	complete := func() { s.reg.Done(&RegisterResult{State: s.new}) }
@@ -353,9 +345,7 @@ func NewDeleteReplacementStep(
 	// that it can issue a deletion of this resource on the next update to this stack.
 	contract.Assertf(pendingReplace != old.Delete,
 		"resource %v cannot be pending replacement and deletion at the same time", old.URN)
-	old.Lock.Lock()
 	old.PendingReplacement = pendingReplace
-	old.Lock.Unlock()
 	return &DeleteStep{
 		deployment:     deployment,
 		otherDeletions: otherDeletions,
@@ -531,9 +521,6 @@ func (s *UpdateStep) Diffs() []resource.PropertyKey                { return s.di
 func (s *UpdateStep) DetailedDiff() map[string]plugin.PropertyDiff { return s.detailedDiff }
 
 func (s *UpdateStep) Apply(preview bool) (resource.Status, StepCompleteFunc, error) {
-	s.new.Lock.Lock()
-	defer s.new.Lock.Unlock()
-
 	// Always propagate the ID and timestamps even in previews and refreshes.
 	s.new.ID = s.old.ID
 	s.new.Created = s.old.Created
@@ -721,9 +708,6 @@ func (s *ReadStep) Res() *resource.State    { return s.new }
 func (s *ReadStep) Logical() bool           { return !s.replacing }
 
 func (s *ReadStep) Apply(preview bool) (resource.Status, StepCompleteFunc, error) {
-	s.new.Lock.Lock()
-	defer s.new.Lock.Unlock()
-
 	urn := s.new.URN
 	id := s.new.ID
 
@@ -1020,9 +1004,6 @@ func (s *ImportStep) Diffs() []resource.PropertyKey                { return s.di
 func (s *ImportStep) DetailedDiff() map[string]plugin.PropertyDiff { return s.detailedDiff }
 
 func (s *ImportStep) Apply(preview bool) (resource.Status, StepCompleteFunc, error) {
-	s.new.Lock.Lock()
-	defer s.new.Lock.Unlock()
-
 	complete := func() {
 		s.reg.Done(&RegisterResult{State: s.new})
 	}

pkg/resource/deploy/step_executor.go

@@ -257,9 +257,7 @@ func (se *stepExecutor) executeRegisterResourceOutputs(
 		}
 	}
 
-	reg.New().Lock.Lock()
 	reg.New().Outputs = outs
-	reg.New().Lock.Unlock()
 
 	// If we're generating plans save these new outputs to the plan
 	if se.opts.GeneratePlan {
@@ -422,8 +420,6 @@ func (se *stepExecutor) executeStep(workerID int, step Step) error {
 	// https://github.com/pulumi/pulumi/issues/14994).
 	if step.New() != nil && step.Op() != OpReplace {
 		newState := step.New()
-		newState.Lock.Lock()
-
 		for _, k := range newState.AdditionalSecretOutputs {
 			if k == "id" {
 				se.deployment.Diag().Warningf(&diag.Diag{
@@ -468,8 +464,6 @@ func (se *stepExecutor) executeStep(workerID int, step Step) error {
 			}
 		}
 
-		newState.Lock.Unlock()
-
 		// If this is not a resource that is managed by Pulumi, then we can ignore it.
 		if _, hasGoal := se.deployment.goals.Load(newState.URN); hasGoal {
 			se.deployment.news.Store(newState.URN, newState)

pkg/resource/stack/deployment.go

@@ -308,9 +308,6 @@ func SerializeResource(
 	contract.Requiref(res != nil, "res", "must not be nil")
 	contract.Requiref(res.URN != "", "res", "must have a URN")
 
-	res.Lock.Lock()
-	defer res.Lock.Unlock()
-
 	// Serialize all input and output properties recursively, and add them if non-empty.
 	var inputs map[string]interface{}
 	if inp := res.Inputs; inp != nil {

sdk/go/common/resource/resource_state.go

@@ -15,7 +15,6 @@
 package resource
 
 import (
-	"sync"
 	"time"
 
 	"github.com/pulumi/pulumi/sdk/v3/go/common/tokens"
@@ -28,11 +27,6 @@ import (
 //
 //nolint:lll
 type State struct {
-	// Currently the engine implements RegisterResourceOutputs by directly mutating the state to change the `Outputs`. This
-	// triggers a race between the snapshot serialization code and the engine. Ideally we'd do a more principled fix, but
-	// just locking in these two places is sufficient to stop the race detector from firing on integration tests.
-	Lock sync.Mutex
-
 	Type                    tokens.Type           // the resource's type.
 	URN                     URN                   // the resource's object urn, a human-friendly, unique name for the resource.
 	Custom                  bool                  // true if the resource is custom, managed by a plugin.
@@ -59,36 +53,6 @@ type State struct {
 	SourcePosition          string                // If set, the source location of the resource registration
 }
 
-// Copy creates a deep copy of the resource state, except without copying the lock.
-func (s *State) Copy() *State {
-	return &State{
-		Type:                    s.Type,
-		URN:                     s.URN,
-		Custom:                  s.Custom,
-		Delete:                  s.Delete,
-		ID:                      s.ID,
-		Inputs:                  s.Inputs,
-		Outputs:                 s.Outputs,
-		Parent:                  s.Parent,
-		Protect:                 s.Protect,
-		External:                s.External,
-		Dependencies:            s.Dependencies,
-		InitErrors:              s.InitErrors,
-		Provider:                s.Provider,
-		PropertyDependencies:    s.PropertyDependencies,
-		PendingReplacement:      s.PendingReplacement,
-		AdditionalSecretOutputs: s.AdditionalSecretOutputs,
-		Aliases:                 s.Aliases,
-		CustomTimeouts:          s.CustomTimeouts,
-		ImportID:                s.ImportID,
-		RetainOnDelete:          s.RetainOnDelete,
-		DeletedWith:             s.DeletedWith,
-		Created:                 s.Created,
-		Modified:                s.Modified,
-		SourcePosition:          s.SourcePosition,
-	}
-}
-
 func (s *State) GetAliasURNs() []URN {
 	return s.Aliases
 }