fix(discovery): prevent resources from becoming unmanaged after sync

Author: browdues

internal/metastructure/discovery/discovery.go

@@ -485,17 +485,20 @@ func synchronizeResources(op ListOperation, namespace string, target pkgmodel.Ta
 
 	resourceFilters := (*plugin).GetResourceFilters()
 
-	var resourcesToSynchronize []pkgmodel.Resource
+	var nativeIDs []string
 	for _, resource := range resources {
-		// We initially set the label to the native ID. Since not every List API reliably returns tags we need to overwrite the label
-		// in the resource_updater where we sync (read) the resource.
+		nativeIDs = append(nativeIDs, resource.NativeID)
+	}
+
+	var resourcesToSynchronize []pkgmodel.Resource
+	for _, nativeID := range nativeIDs {
 		resourcesToSynchronize = append(resourcesToSynchronize, pkgmodel.Resource{
-			Label:      url.QueryEscape(resource.NativeID),
+			Label:      url.QueryEscape(nativeID),
 			Type:       op.ResourceType,
 			Stack:      constants.UnmanagedStack,
 			Target:     target.Label,
-			NativeID:   resource.NativeID,
-			Properties: injectResolvables(resource.Properties, op),
+			NativeID:   nativeID,
+			Properties: injectResolvables("{}", op),
 			Schema:     schema,
 			Managed:    false,
 			Ksuid:      util.NewID(),

internal/metastructure/resource_persister/resource_persister.go

@@ -329,6 +329,13 @@ func (rp *ResourcePersister) processResourceUpdate(commandID string, stack pkgmo
 			if !util.JsonEqualRaw(currentResource.Properties, rc.Resource.Properties) ||
 				!util.JsonEqualRaw(currentResource.ReadOnlyProperties, rc.Resource.ReadOnlyProperties) {
 
+				// Preserve the current stack and managed state during sync READ operations
+				// to prevent stale sync data from overwriting recent stack changes
+				secretSafeResource.Stack = currentResource.Stack
+				secretSafeResource.Managed = currentResource.Managed
+				secretSafeResource.Schema.Discoverable = currentResource.Schema.Discoverable
+				secretSafeResource.Schema.Extractable = currentResource.Schema.Extractable
+
 				currentResource.Properties = secretSafeResource.Properties
 				currentResource.ReadOnlyProperties = secretSafeResource.ReadOnlyProperties
 

internal/metastructure/resource_persister/resource_persister_test.go

@@ -982,6 +982,96 @@ func TestResourcePersister_ValidateFields(t *testing.T) {
 	})
 }
 
+func TestResourcePersister_ReadPreservesCurrentStack(t *testing.T) {
+	persister, sender, ds, err := newResourcePersisterForTest(t)
+	assert.NoError(t, err)
+
+	resourceKsuid := util.NewID()
+
+	initialResource := resource_update.ResourceUpdate{
+		Resource: pkgmodel.Resource{
+			Label:      "test-vpc",
+			Type:       "AWS::EC2::VPC",
+			Properties: json.RawMessage(`{"CidrBlock":"10.0.0.0/16"}`),
+			Stack:      "managed-stack",
+			Ksuid:      resourceKsuid,
+			Managed:    true,
+		},
+		ResourceTarget: pkgmodel.Target{
+			Label:     "test-target",
+			Namespace: "aws",
+		},
+		State:      resource_update.ResourceUpdateStateSuccess,
+		StackLabel: "managed-stack",
+		ProgressResult: []resource.ProgressResult{
+			{
+				Operation:          resource.OperationCreate,
+				OperationStatus:    resource.OperationStatusSuccess,
+				NativeID:           "vpc-123",
+				ResourceType:       "AWS::EC2::VPC",
+				ResourceProperties: json.RawMessage(`{"CidrBlock":"10.0.0.0/16"}`),
+				StartTs:            util.TimeNow(),
+				ModifiedTs:         util.TimeNow(),
+			},
+		},
+	}
+
+	createResult := persister.Call(sender, resource_update.PersistResourceUpdate{
+		CommandID:         "create-cmd",
+		ResourceOperation: resource_update.OperationCreate,
+		PluginOperation:   resource.OperationCreate,
+		ResourceUpdate:    initialResource,
+	})
+	assert.NoError(t, createResult.Error)
+
+	syncUpdate := resource_update.ResourceUpdate{
+		Resource: pkgmodel.Resource{
+			Label:      "test-vpc",
+			Type:       "AWS::EC2::VPC",
+			Properties: json.RawMessage(`{"CidrBlock":"10.0.0.0/16","VpcId":"vpc-123"}`),
+			Stack:      "$unmanaged",
+			Ksuid:      resourceKsuid,
+			Managed:    false,
+		},
+		ResourceTarget: pkgmodel.Target{
+			Label:     "test-target",
+			Namespace: "aws",
+		},
+		State:      resource_update.ResourceUpdateStateSuccess,
+		StackLabel: "$unmanaged",
+		ProgressResult: []resource.ProgressResult{
+			{
+				Operation:          resource.OperationRead,
+				OperationStatus:    resource.OperationStatusSuccess,
+				NativeID:           "vpc-123",
+				ResourceType:       "AWS::EC2::VPC",
+				ResourceProperties: json.RawMessage(`{"CidrBlock":"10.0.0.0/16","VpcId":"vpc-123"}`),
+				StartTs:            util.TimeNow(),
+				ModifiedTs:         util.TimeNow(),
+			},
+		},
+	}
+
+	readResult := persister.Call(sender, resource_update.PersistResourceUpdate{
+		CommandID:         "sync-cmd",
+		ResourceOperation: resource_update.OperationRead,
+		PluginOperation:   resource.OperationRead,
+		ResourceUpdate:    syncUpdate,
+	})
+	assert.NoError(t, readResult.Error)
+
+	loadedStack, err := ds.LoadStack("managed-stack")
+	assert.NoError(t, err)
+	assert.NotNil(t, loadedStack, "Resource should remain in managed-stack, not move to $unmanaged")
+	assert.Equal(t, 1, len(loadedStack.Resources))
+	assert.Equal(t, "test-vpc", loadedStack.Resources[0].Label)
+	assert.True(t, loadedStack.Resources[0].Managed, "Resource should remain managed")
+
+	unmanagedStack, err := ds.LoadStack("$unmanaged")
+	assert.NoError(t, err)
+	assert.Nil(t, unmanagedStack, "Resource should not appear in $unmanaged stack")
+}
+
 // newResourcePersisterForTest creates a ResourcePersister actor for testing.
 // This follows the same pattern as FormaCommandPersister tests.
 func newResourcePersisterForTest(t *testing.T) (*unit.TestActor, gen.PID, datastore.Datastore, error) {

internal/workflow_tests/discovery_test.go

@@ -782,3 +782,62 @@ func TestDiscovery_NoDiscoverableTargets_CompletesImmediately(t *testing.T) {
 		assert.Nil(t, stack, "No unmanaged stack should be created when no discoverable targets exist")
 	})
 }
+
+func TestDiscovery_ListPropertiesNotPersistedOnlyReadProperties(t *testing.T) {
+	testutil.RunTestFromProjectRoot(t, func(t *testing.T) {
+		overrides := &plugin.ResourcePluginOverrides{
+			List: func(req *resource.ListRequest) (*resource.ListResult, error) {
+				if req.ResourceType != "FakeAWS::S3::Bucket" {
+					return &resource.ListResult{}, nil
+				}
+				return &resource.ListResult{
+					Resources: []resource.Resource{
+						{
+							NativeID:   "test-bucket",
+							Properties: `{"ListOnlyProp": "should-not-persist", "BucketName": "list-value"}`,
+						},
+					},
+				}, nil
+			},
+			Read: func(req *resource.ReadRequest) (*resource.ReadResult, error) {
+				return &resource.ReadResult{
+					ResourceType: "FakeAWS::S3::Bucket",
+					Properties:   `{"BucketName": "read-value", "ReadOnlyProp": "from-read"}`,
+				}, nil
+			},
+		}
+
+		cfg := test_helpers.NewTestMetastructureConfig()
+		m, def, err := test_helpers.NewTestMetastructureWithConfig(t, overrides, cfg)
+		defer def()
+		require.NoError(t, err)
+
+		target := &pkgmodel.Target{
+			Label:        "us-east-1",
+			Namespace:    "FakeAWS",
+			Config:       json.RawMessage(`{"region":"us-east-1"}`),
+			Discoverable: true,
+		}
+		_, err = m.Datastore.CreateTarget(target)
+		require.NoError(t, err)
+
+		_, err = testutil.StartTestHelperActor(m.Node, make(chan any, 1))
+		require.NoError(t, err)
+
+		err = testutil.Send(m.Node, "Discovery", discovery.Discover{})
+		require.NoError(t, err)
+
+		var stack *pkgmodel.Forma
+		require.Eventually(t, func() bool {
+			stack, err = m.Datastore.LoadStack("$unmanaged")
+			return err == nil && stack != nil && len(stack.Resources) == 1
+		}, 5*time.Second, 100*time.Millisecond)
+
+		res := stack.Resources[0]
+		var props map[string]any
+		require.NoError(t, json.Unmarshal(res.Properties, &props))
+
+		assert.Equal(t, "read-value", props["BucketName"], "BucketName should come from READ, not LIST")
+		assert.NotContains(t, props, "ListOnlyProp", "LIST-only properties should not be persisted")
+	})
+}

internal/workflow_tests/synchronizer_test.go

@@ -804,3 +804,101 @@ func TestSynchronizer_ExcludesResourcesBeingUpdatedByApply(t *testing.T) {
 		}
 	})
 }
+
+// TestSynchronizer_SyncDoesNotOverwriteApplyStackChange verifies that sync preserves
+// user-controlled state (Stack, Managed, Schema.Discoverable, Schema.Extractable) when
+// a race occurs between apply and sync operations.
+func TestSynchronizer_SyncDoesNotOverwriteApplyStackChange(t *testing.T) {
+	testutil.RunTestFromProjectRoot(t, func(t *testing.T) {
+		readStarted := make(chan struct{})
+		readCanComplete := make(chan struct{})
+		readCount := 0
+		var mu sync.Mutex
+
+		nativeID := "vpc-" + uuid.New().String()
+
+		overrides := &plugin.ResourcePluginOverrides{
+			Read: func(request *resource.ReadRequest) (*resource.ReadResult, error) {
+				mu.Lock()
+				readCount++
+				currentRead := readCount
+				mu.Unlock()
+
+				if currentRead == 1 {
+					select {
+					case readStarted <- struct{}{}:
+					default:
+					}
+					<-readCanComplete
+				}
+
+				return &resource.ReadResult{
+					ResourceType: request.ResourceType,
+					Properties:   `{"CidrBlock":"10.0.0.0/16","VpcId":"` + nativeID + `"}`,
+				}, nil
+			},
+		}
+
+		cfg := test_helpers.NewTestMetastructureConfig()
+		cfg.Agent.Synchronization.Enabled = false
+		m, def, err := test_helpers.NewTestMetastructureWithConfig(t, overrides, cfg)
+		defer def()
+		require.NoError(t, err)
+
+		unmanagedResource := &pkgmodel.Resource{
+			Ksuid:      util.NewID(),
+			NativeID:   nativeID,
+			Stack:      "$unmanaged",
+			Type:       "FakeAWS::EC2::VPC",
+			Label:      nativeID,
+			Target:     "test-target",
+			Properties: json.RawMessage(`{"CidrBlock":"10.0.0.0/16"}`),
+			Schema:     pkgmodel.Schema{Discoverable: false, Extractable: false},
+			Managed:    false,
+		}
+		_, err = m.Datastore.StoreResource(unmanagedResource, "discovery-cmd")
+		require.NoError(t, err)
+
+		_, err = m.Datastore.CreateTarget(&pkgmodel.Target{Label: "test-target", Discoverable: true})
+		require.NoError(t, err)
+
+		go func() { _ = m.ForceSync() }()
+		<-readStarted
+
+		managedStack := "my-managed-stack"
+		f := &pkgmodel.Forma{
+			Stacks: []pkgmodel.Stack{{Label: managedStack}},
+			Resources: []pkgmodel.Resource{{
+				Ksuid:      unmanagedResource.Ksuid,
+				Label:      nativeID,
+				Type:       "FakeAWS::EC2::VPC",
+				NativeID:   nativeID,
+				Properties: json.RawMessage(`{"CidrBlock":"10.0.0.0/16"}`),
+				Stack:      managedStack,
+				Schema:     pkgmodel.Schema{Fields: []string{"CidrBlock"}, Discoverable: true, Extractable: true},
+				Target:     "test-target",
+				Managed:    true,
+			}},
+			Targets: []pkgmodel.Target{},
+		}
+		_, err = m.ApplyForma(f, &config.FormaCommandConfig{Mode: pkgmodel.FormaApplyModeReconcile}, "test")
+		require.NoError(t, err)
+
+		require.Eventually(t, func() bool {
+			res, err := m.Datastore.LoadResourceByNativeID(nativeID, "FakeAWS::EC2::VPC")
+			return err == nil && res != nil && res.Stack == managedStack && res.Managed
+		}, 5*time.Second, 100*time.Millisecond)
+
+		// Unblock sync's READ
+		close(readCanComplete)
+		time.Sleep(2 * time.Second)
+
+		res, err := m.Datastore.LoadResourceByNativeID(nativeID, "FakeAWS::EC2::VPC")
+		require.NoError(t, err)
+		require.NotNil(t, res)
+		assert.Equal(t, managedStack, res.Stack, "Stack should be preserved")
+		assert.True(t, res.Managed, "Managed should be preserved")
+		assert.True(t, res.Schema.Discoverable, "Schema.Discoverable should be preserved")
+		assert.True(t, res.Schema.Extractable, "Schema.Extractable should be preserved")
+	})
+}