Better handle property dependencies and deletedWith

A resource `A` depends on a resource `B` if:

1. `B` is `A`'s `Provider`.
2. `B` is `A`'s `Parent`.
3. `B` appears in `A`'s `Dependencies`.
4. `B` appears in one or more of `A`'s `PropertyDependencies`.
5. `B` is referenced by `A`'s `DeletedWith` field.

While cases 1, 2, and 3 (providers, parents, and dependencies) are
handled fairly consistently, there have been a number of cases where the
newer features of `PropertyDependencies` (case 4) and `DeletedWith`
(case 5) have been neglected. This commit addresses some of these
omissions. Specifically:

* When refreshing state, When refreshing state, it's important that we
  remove URNs that point to resources that we've identified as deleted.
  Presently we check pointers to parents and dependencies, but not
  property dependencies or `deletedWith`. This commit fixes these gaps
  where dangling URNs could lurk.

* Linked to the above, this commit extends snapshot integrity checks to
  include property dependencies and `deletedWith`. Some tests that
  previously used now invalid states have to be repaired or removed as a
  result of this.

* Fixing snapshot integrity checking reveals that dependency graph
  checks also fail to consider property dependencies and `deletedWith`.
  This probably hasn't bitten us since property dependencies are
  currently rolled up into dependencies (for temporary backwards
  compatibility) and `deletedWith` is typically an optimisation
  (moreover, one that only matters during deletes), so operations being
  parallelised due a perceived lack of dependency still succeed.
  However, tests that previously passed now fail as we can spot these
  races with our better integrity checks. This commit thus fixes up
  dependency graph construction and bulks out its test suite to
  cover the new cases.

changelog/pending/20240430--engine--property-dependencies-deletedwith.yaml

@@ -0,0 +1,4 @@
+changes:
+- type: fix
+ scope: engine
+ description: Better handle property dependencies and deleted-with relationships when pruning URNs, verifying snapshot integrity and computing dependency graphs.

pkg/engine/lifecycletest/provider_test.go

@@ -1510,11 +1510,14 @@ func TestProviderVersionAssignment(t *testing.T) {
 func TestDeletedWithOptionInheritance(t *testing.T) {
  t.Parallel()
 
- expectedUrn := resource.CreateURN("expect-this", "pkg:index:type", "", "project", "stack")
+ var deletionDepURN resource.URN
 
  programF := deploytest.NewLanguageRuntimeF(func(_ plugin.RunInfo, monitor *deploytest.ResourceMonitor) error {
+ deletionDep, err := monitor.RegisterResource("pkgA:m:typA", "deletable", false)
+ assert.NoError(t, err)
+
  parentResp, err := monitor.RegisterResource("pkgA:m:typA", "resA", true, deploytest.ResourceOptions{
- DeletedWith: expectedUrn,
+ DeletedWith: deletionDep.URN,
  })
  assert.NoError(t, err)
 
@@ -1523,6 +1526,7 @@ func TestDeletedWithOptionInheritance(t *testing.T) {
  })
  assert.NoError(t, err)
 
+ deletionDepURN = deletionDep.URN
  return nil
  })
 
@@ -1546,8 +1550,8 @@ func TestDeletedWithOptionInheritance(t *testing.T) {
 
  project := p.GetProject()
  snap, err := TestOp(Update).Run(project, p.GetTarget(t, nil), p.Options, false, p.BackendClient, nil)
- for _, res := range snap.Resources[1:] {
- assert.Equal(t, expectedUrn, res.DeletedWith)
+ for _, res := range snap.Resources[2:] {
+ assert.Equal(t, deletionDepURN, res.DeletedWith)
  }
  assert.NoError(t, err)
 }
@@ -1558,12 +1562,15 @@ func TestDeletedWithOptionInheritance(t *testing.T) {
 func TestDeletedWithOptionInheritanceMLC(t *testing.T) {
  t.Parallel()
 
- expectedUrn := resource.CreateURN("expect-this", "pkg:index:type", "", "project", "stack")
+ var deletionDepURN resource.URN
 
  programF := deploytest.NewLanguageRuntimeF(func(_ plugin.RunInfo, monitor *deploytest.ResourceMonitor) error {
+ deletionDep, err := monitor.RegisterResource("pkgA:m:typComponent", "deletable", false)
+ assert.NoError(t, err)
+
  parentResp, err := monitor.RegisterResource("pkgA:m:typComponent", "resA", false, deploytest.ResourceOptions{
  Remote: true,
- DeletedWith: expectedUrn,
+ DeletedWith: deletionDep.URN,
  })
  assert.NoError(t, err)
 
@@ -1572,6 +1579,7 @@ func TestDeletedWithOptionInheritanceMLC(t *testing.T) {
  })
  assert.NoError(t, err)
 
+ deletionDepURN = deletionDep.URN
  return nil
  })
 
@@ -1615,8 +1623,8 @@ func TestDeletedWithOptionInheritanceMLC(t *testing.T) {
 
  project := p.GetProject()
  snap, err := TestOp(Update).Run(project, p.GetTarget(t, nil), p.Options, false, p.BackendClient, nil)
- for _, res := range snap.Resources[1:] {
- assert.Equal(t, expectedUrn, res.DeletedWith)
+ for _, res := range snap.Resources[2:] {
+ assert.Equal(t, deletionDepURN, res.DeletedWith)
  }
  assert.NoError(t, err)
 }

pkg/engine/lifecycletest/pulumi_test.go

@@ -3466,107 +3466,6 @@ func TestDeletedWith(t *testing.T) {
  assert.Len(t, snap.Resources, 0)
 }
 
-func TestDeletedWithCircularDependency(t *testing.T) {
- // This test should be removed if DeletedWith circular dependency is taken care of.
- // At the mean time, if there is a circular dependency - none shall be deleted.
- t.Parallel()
-
- idCounter := 0
-
- loaders := []*deploytest.ProviderLoader{
- deploytest.NewProviderLoader("pkgA", semver.MustParse("1.0.0"), func() (plugin.Provider, error) {
- return &deploytest.Provider{
- DiffF: func(
- urn resource.URN,
- id resource.ID,
- oldInputs, oldOutputs, newInputs resource.PropertyMap,
- ignoreChanges []string,
- ) (plugin.DiffResult, error) {
- return plugin.DiffResult{}, nil
- },
- CreateF: func(urn resource.URN, news resource.PropertyMap, timeout float64,
- preview bool,
- ) (resource.ID, resource.PropertyMap, resource.Status, error) {
- resourceID := resource.ID(fmt.Sprintf("created-id-%d", idCounter))
- idCounter = idCounter + 1
- return resourceID, news, resource.StatusOK, nil
- },
- DeleteF: func(urn resource.URN, id resource.ID, oldInputs, oldOutputs resource.PropertyMap,
- timeout float64,
- ) (resource.Status, error) {
- assert.Fail(t, "Delete was called")
-
- return resource.StatusOK, nil
- },
- }, nil
- }, deploytest.WithoutGrpc),
- }
-
- ins := resource.NewPropertyMapFromMap(map[string]interface{}{
- "foo": "bar",
- })
-
- createResource := true
- cURN := resource.URN("")
-
- programF := deploytest.NewLanguageRuntimeF(func(_ plugin.RunInfo, monitor *deploytest.ResourceMonitor) error {
- if createResource {
- respA, err := monitor.RegisterResource("pkgA:m:typA", "resA", true, deploytest.ResourceOptions{
- Inputs: ins,
- DeletedWith: cURN,
- })
- assert.NoError(t, err)
-
- respB, err := monitor.RegisterResource("pkgA:m:typA", "resB", true, deploytest.ResourceOptions{
- Inputs: ins,
- DeletedWith: respA.URN,
- })
- assert.NoError(t, err)
-
- respC, err := monitor.RegisterResource("pkgA:m:typA", "resC", true, deploytest.ResourceOptions{
- Inputs: ins,
- DeletedWith: respB.URN,
- })
- cURN = respC.URN
- assert.NoError(t, err)
- }
-
- return nil
- })
- hostF := deploytest.NewPluginHostF(nil, nil, programF, loaders...)
-
- p := &TestPlan{
- Options: TestUpdateOptions{HostF: hostF},
- }
-
- project := p.GetProject()
-
- // Run an update to create the resource
- snap, err := TestOp(Update).Run(project, p.GetTarget(t, nil), p.Options, false, p.BackendClient, nil)
- assert.NoError(t, err)
- assert.NotNil(t, snap)
- assert.Len(t, snap.Resources, 4)
- assert.Equal(t, "created-id-0", snap.Resources[1].ID.String())
- assert.Equal(t, "created-id-1", snap.Resources[2].ID.String())
- assert.Equal(t, "created-id-2", snap.Resources[3].ID.String())
-
- // Run again to update DeleteWith for resA
- snap, err = TestOp(Update).Run(project, p.GetTarget(t, snap), p.Options, false, p.BackendClient, nil)
- assert.NoError(t, err)
- assert.NotNil(t, snap)
- assert.Len(t, snap.Resources, 4)
- assert.Equal(t, "created-id-0", snap.Resources[1].ID.String())
- assert.Equal(t, "created-id-1", snap.Resources[2].ID.String())
- assert.Equal(t, "created-id-2", snap.Resources[3].ID.String())
-
- // Run a new update which will cause a delete, we still shouldn't see a provider delete
- createResource = false
- snap, err = TestOp(Update).Run(project, p.GetTarget(t, snap), p.Options, false, p.BackendClient, nil)
- assert.NoError(t, err)
- assert.NotNil(t, snap)
- assert.Len(t, snap.Resources, 0)
-}
-
 func TestInvalidGetIDReportsUserError(t *testing.T) {
  t.Parallel()
 

pkg/engine/lifecycletest/refresh_test.go

@@ -304,6 +304,126 @@ func TestRefreshDeleteDependencies(t *testing.T) {
  }
 }
 
+func TestRefreshDeletePropertyDependencies(t *testing.T) {
+ t.Parallel()
+
+ loaders := []*deploytest.ProviderLoader{
+ deploytest.NewProviderLoader("pkgA", semver.MustParse("1.0.0"), func() (plugin.Provider, error) {
+ return &deploytest.Provider{
+ ReadF: func(
+ urn resource.URN, id resource.ID, inputs, state resource.PropertyMap,
+ ) (plugin.ReadResult, resource.Status, error) {
+ if urn.Name() == "resA" {
+ return plugin.ReadResult{}, resource.StatusOK, nil
+ }
+
+ return plugin.ReadResult{Outputs: resource.PropertyMap{}}, resource.StatusOK, nil
+ },
+ }, nil
+ }),
+ }
+
+ programF := deploytest.NewLanguageRuntimeF(func(_ plugin.RunInfo, monitor *deploytest.ResourceMonitor) error {
+ resA, err := monitor.RegisterResource("pkgA:m:typA", "resA", true)
+ assert.NoError(t, err)
+
+ _, err = monitor.RegisterResource("pkgA:m:typA", "resB", true, deploytest.ResourceOptions{
+ PropertyDeps: map[resource.PropertyKey][]resource.URN{
+ "propB1": {resA.URN},
+ },
+ })
+
+ assert.NoError(t, err)
+ return nil
+ })
+
+ hostF := deploytest.NewPluginHostF(nil, nil, programF, loaders...)
+
+ p := &TestPlan{Options: TestUpdateOptions{HostF: hostF}}
+
+ p.Steps = []TestStep{{Op: Update}}
+ snap := p.Run(t, nil)
+
+ assert.Len(t, snap.Resources, 3)
+ assert.Equal(t, snap.Resources[0].URN.Name(), "default") // provider
+ assert.Equal(t, snap.Resources[1].URN.Name(), "resA")
+ assert.Equal(t, snap.Resources[2].URN.Name(), "resB")
+ assert.Equal(t, snap.Resources[2].PropertyDependencies["propB1"][0].Name(), "resA")
+
+ err := snap.VerifyIntegrity()
+ assert.NoError(t, err)
+
+ p.Steps = []TestStep{{Op: Refresh}}
+ snap = p.Run(t, snap)
+
+ assert.Len(t, snap.Resources, 2)
+ assert.Equal(t, snap.Resources[0].URN.Name(), "default") // provider
+ assert.Equal(t, snap.Resources[1].URN.Name(), "resB")
+ assert.Empty(t, snap.Resources[1].PropertyDependencies)
+
+ err = snap.VerifyIntegrity()
+ assert.NoError(t, err)
+}
+
+func TestRefreshDeleteDeletedWithDependencies(t *testing.T) {
+ t.Parallel()
+
+ loaders := []*deploytest.ProviderLoader{
+ deploytest.NewProviderLoader("pkgA", semver.MustParse("1.0.0"), func() (plugin.Provider, error) {
+ return &deploytest.Provider{
+ ReadF: func(
+ urn resource.URN, id resource.ID, inputs, state resource.PropertyMap,
+ ) (plugin.ReadResult, resource.Status, error) {
+ if urn.Name() == "resA" {
+ return plugin.ReadResult{}, resource.StatusOK, nil
+ }
+
+ return plugin.ReadResult{Outputs: resource.PropertyMap{}}, resource.StatusOK, nil
+ },
+ }, nil
+ }),
+ }
+
+ programF := deploytest.NewLanguageRuntimeF(func(_ plugin.RunInfo, monitor *deploytest.ResourceMonitor) error {
+ resA, err := monitor.RegisterResource("pkgA:m:typA", "resA", true)
+ assert.NoError(t, err)
+
+ _, err = monitor.RegisterResource("pkgA:m:typA", "resB", true, deploytest.ResourceOptions{
+ DeletedWith: resA.URN,
+ })
+
+ assert.NoError(t, err)
+ return nil
+ })
+
+ hostF := deploytest.NewPluginHostF(nil, nil, programF, loaders...)
+
+ p := &TestPlan{Options: TestUpdateOptions{HostF: hostF}}
+
+ p.Steps = []TestStep{{Op: Update}}
+ snap := p.Run(t, nil)
+
+ assert.Len(t, snap.Resources, 3)
+ assert.Equal(t, snap.Resources[0].URN.Name(), "default") // provider
+ assert.Equal(t, snap.Resources[1].URN.Name(), "resA")
+ assert.Equal(t, snap.Resources[2].URN.Name(), "resB")
+ assert.Equal(t, snap.Resources[2].DeletedWith.Name(), "resA")
+
+ err := snap.VerifyIntegrity()
+ assert.NoError(t, err)
+
+ p.Steps = []TestStep{{Op: Refresh}}
+ snap = p.Run(t, snap)
+
+ assert.Len(t, snap.Resources, 2)
+ assert.Equal(t, snap.Resources[0].URN.Name(), "default") // provider
+ assert.Equal(t, snap.Resources[1].URN.Name(), "resB")
+ assert.Empty(t, snap.Resources[1].DeletedWith)
+
+ err = snap.VerifyIntegrity()
+ assert.NoError(t, err)
+}
+
 // Looks up the provider ID in newResources and sets "Provider" to reference that in every resource in oldResources.
 func setProviderRef(t *testing.T, oldResources, newResources []*resource.State, provURN resource.URN) {
  for _, r := range newResources {

pkg/resource/deploy/deployment_executor.go

@@ -645,6 +645,31 @@ func (ex *deploymentExecutor) rebuildBaseState(resourceToStep map[*resource.Stat
  new.Dependencies = deps
  }
 
+ // Remove any deleted resources from this resource's property dependencies
+ // lists. If we end up emptying a property dependency list, we'll remove the
+ // property from the map altogether.
+ for prop, deps := range new.PropertyDependencies {
+ if len(deps) != 0 {
+ newDeps := slice.Prealloc[resource.URN](len(deps))
+ for _, d := range deps {
+ if referenceable[d] {
+ newDeps = append(newDeps, d)
+ }
+ }
+
+ if len(newDeps) > 0 {
+ new.PropertyDependencies[prop] = newDeps
+ } else {
+ delete(new.PropertyDependencies, prop)
+ }
+ }
+ }
+
+ // Remove any deleted resources from DeletedWith properties.
+ if new.DeletedWith != "" && !referenceable[new.DeletedWith] {
+ new.DeletedWith = ""
+ }
+
  // Add this resource to the resource list and mark it as referenceable.
  resources = append(resources, new)
  referenceable[new.URN] = true