fix(datastore): preserve staged deletes across Merge composition (#1044)

`MemoryDataStore.Merge` propagated `src.DeletedRemoteKeys` by calling
`dst.<Store>.Delete(key)`, which removed the record from `dst.Records`
but never appended the key to `dst.DeletedRemoteKeys`. Chained Merges
(operation and changeset composition) consumed the prior step's staged
deletes at every hop; both the JSON file and catalog persistence
backends drive remote deletes off DeletedRemoteKeys, so any delete that
passed through more than one Merge silently never reached the remote.

Each of the three propagation blocks (`AddressRef`, `ChainMetadata`,
`ContractMetadata`) now calls `dst.<Store>.RemoteDelete(key)` followed
by `dst.<Store>.Delete(key)` with the per-store `NotFound` sentinel
tolerated. Delete intent now survives chained Merges, and Merge is
idempotent on repeated calls with the same source.

Behavior change: Merge previously returned Err<X>NotFound when the
source staged a delete for a key not present in `dst.Records`; it now
succeeds and stages the delete on dst. This aligns observable behavior
with the RemoteDelete docstring, which already advertised staging
deletes for records not held locally. Three existing tests in
`memory_datastore_test.go` are inverted accordingly; new tests cover
chained-merge preservation, idempotency, and the precedence of
source-live records over destination-staged deletes.

Author: giogam

.changeset/large-wombats-fall.md

@@ -0,0 +1,5 @@
+---
+"chainlink-deployments-framework": patch
+---
+
+fix(datastore): preserve staged deletes across Merge composition

datastore/memory_datastore.go

@@ -62,7 +62,26 @@ func (s *MemoryDataStore) WriteMetadata(bundle MetadataBundle, opts ...WriteMeta
 	return WriteMetadataToDataStore(s, bundle, opts...)
 }
 
-// Merge merges the given mutable data store into the current MemoryDataStore.
+// Merge applies records and staged deletions from other onto this MemoryDataStore.
+//
+// Staged deletions (other.<Store>.DeletedRemoteKeys) are propagated by appending the
+// key to s.<Store>.DeletedRemoteKeys via RemoteDelete and then removing the record
+// from s.<Store>.Records via Delete (tolerating the per-store NotFound sentinel).
+// The DeletedRemoteKeys append is what lets chained operations preserve delete intent
+// across intermediate Merges.
+//
+// NotFound on the Delete step is tolerated because the source's staged key may
+// legitimately not be present in the destination's Records (RemoteDelete is allowed
+// to stage deletes for records that exist only in the remote backing store), and
+// because we want repeated Merges of the same source to be idempotent.
+//
+// Precedence: a live record in other.<Store>.Records overrides a staged delete in
+// s.<Store>.DeletedRemoteKeys for the same key. This is a side-effect of Upsert,
+// which clears the key from DeletedRemoteKeys whenever a record is upserted (so a
+// direct `RemoteDelete(k); Upsert(rec-with-k)` cancels the staged delete on the
+// same store). Staged deletes are only "sticky" on the source side. Callers that
+// need a destination-side staged delete to survive Merge must ensure the source
+// either omits the live record or stages the delete itself.
 func (s *MemoryDataStore) Merge(other DataStore) error {
 	// Fetch address ref records from the other data store
 	addressRefs, err := other.Addresses().Fetch()
@@ -84,7 +103,10 @@ func (s *MemoryDataStore) Merge(other DataStore) error {
 			if keyErr != nil {
 				return fmt.Errorf("failed to parse address ref key: %w", keyErr)
 			}
-			if err = s.AddressRefStore.Delete(key); err != nil {
+			if err = s.AddressRefStore.RemoteDelete(key); err != nil {
+				return err
+			}
+			if err = s.AddressRefStore.Delete(key); err != nil && !errors.Is(err, ErrAddressRefNotFound) {
 				return err
 			}
 		}
@@ -109,7 +131,10 @@ func (s *MemoryDataStore) Merge(other DataStore) error {
 			if keyErr != nil {
 				return fmt.Errorf("failed to parse chain metadata key: %w", keyErr)
 			}
-			if err = s.ChainMetadataStore.Delete(key); err != nil {
+			if err = s.ChainMetadataStore.RemoteDelete(key); err != nil {
+				return err
+			}
+			if err = s.ChainMetadataStore.Delete(key); err != nil && !errors.Is(err, ErrChainMetadataNotFound) {
 				return err
 			}
 		}
@@ -135,7 +160,10 @@ func (s *MemoryDataStore) Merge(other DataStore) error {
 			if keyErr != nil {
 				return fmt.Errorf("failed to parse contract metadata key: %w", keyErr)
 			}
-			if err = s.ContractMetadataStore.Delete(key); err != nil {
+			if err = s.ContractMetadataStore.RemoteDelete(key); err != nil {
+				return err
+			}
+			if err = s.ContractMetadataStore.Delete(key); err != nil && !errors.Is(err, ErrContractMetadataNotFound) {
 				return err
 			}
 		}

datastore/memory_datastore_test.go

@@ -47,6 +47,9 @@ func TestMemoryDataStore_Merge(t *testing.T) {
 		excpectedChainMetadataCount   int
 		expectedContractMetadataCount int
 		expectedError                 error
+		expectedAddressDRK            []string
+		expectedChainMetaDRK          []string
+		expectedContractMetaDRK       []string
 	}{
 		{
 			name: "Merge single address",
@@ -72,7 +75,7 @@ func TestMemoryDataStore_Merge(t *testing.T) {
 			expectedContractMetadataCount: 1,
 		},
 		{
-			name: "Merge deletions errors: delete address ref record that does not exist produces an error",
+			name: "Merge propagate deletions: stages delete for address ref record not present in destination",
 			setup: func() (*MemoryDataStore, *MemoryDataStore) {
 				dataStore1 := NewMemoryDataStore()
 				dataStore2 := NewMemoryDataStore()
@@ -90,17 +93,18 @@ func TestMemoryDataStore_Merge(t *testing.T) {
 				require.NoError(t, err, "Adding env metadata to dataStore1 should not fail")
 
 				// dataStore2 stages a record for deletion that does not exist in dataStore1
-				require.NoError(t, dataStore2.Addresses().RemoteDelete(NewAddressRefKey(0, "typeA", semver.MustParse("1.0.0"), "q")))
+				stagedKey := NewAddressRefKey(0, "typeA", semver.MustParse("1.0.0"), "q")
+				require.NoError(t, dataStore2.Addresses().RemoteDelete(stagedKey))
 
 				return dataStore1, dataStore2
 			},
 			expectedAddrRefsCount:         1,
 			excpectedChainMetadataCount:   1,
 			expectedContractMetadataCount: 1,
-			expectedError:                 ErrAddressRefNotFound,
+			expectedAddressDRK:            []string{NewAddressRefKey(0, "typeA", semver.MustParse("1.0.0"), "q").String()},
 		},
 		{
-			name: "Merge deletions errors: delete chain metadata record that does not exist produces an error",
+			name: "Merge propagate deletions: stages delete for chain metadata record not present in destination",
 			setup: func() (*MemoryDataStore, *MemoryDataStore) {
 				dataStore1 := NewMemoryDataStore()
 				dataStore2 := NewMemoryDataStore()
@@ -118,17 +122,18 @@ func TestMemoryDataStore_Merge(t *testing.T) {
 				require.NoError(t, err, "Adding env metadata to dataStore2 should not fail")
 
 				// dataStore2 stages a record for deletion that does not exist in dataStore1
-				require.NoError(t, dataStore2.ChainMetadata().RemoteDelete(NewChainMetadataKey(10)))
+				stagedKey := NewChainMetadataKey(10)
+				require.NoError(t, dataStore2.ChainMetadata().RemoteDelete(stagedKey))
 
 				return dataStore1, dataStore2
 			},
 			expectedAddrRefsCount:         1,
 			excpectedChainMetadataCount:   1,
 			expectedContractMetadataCount: 1,
-			expectedError:                 ErrChainMetadataNotFound,
+			expectedChainMetaDRK:          []string{NewChainMetadataKey(10).String()},
 		},
 		{
-			name: "Merge deletions errors: delete contract metadata record that does not exist produces an error",
+			name: "Merge propagate deletions: stages delete for contract metadata record not present in destination",
 			setup: func() (*MemoryDataStore, *MemoryDataStore) {
 				dataStore1 := NewMemoryDataStore()
 				dataStore2 := NewMemoryDataStore()
@@ -146,14 +151,15 @@ func TestMemoryDataStore_Merge(t *testing.T) {
 				require.NoError(t, err, "Adding env metadata to dataStore2 should not fail")
 
 				// dataStore2 stages a record for deletion that does not exist in dataStore1
-				require.NoError(t, dataStore2.ContractMetadata().RemoteDelete(NewContractMetadataKey(10, "0x111")))
+				stagedKey := NewContractMetadataKey(10, "0x111")
+				require.NoError(t, dataStore2.ContractMetadata().RemoteDelete(stagedKey))
 
 				return dataStore1, dataStore2
 			},
 			expectedAddrRefsCount:         1,
 			excpectedChainMetadataCount:   1,
 			expectedContractMetadataCount: 1,
-			expectedError:                 ErrContractMetadataNotFound,
+			expectedContractMetaDRK:       []string{NewContractMetadataKey(10, "0x111").String()},
 		},
 		{
 			name: "Merge propagate deletions: deletes record from remote data store",
@@ -183,6 +189,9 @@ func TestMemoryDataStore_Merge(t *testing.T) {
 			expectedAddrRefsCount:         0,
 			excpectedChainMetadataCount:   0,
 			expectedContractMetadataCount: 0,
+			expectedAddressDRK:            []string{addressRefRecord.Key().String()},
+			expectedChainMetaDRK:          []string{chainMetadataRecord.Key().String()},
+			expectedContractMetaDRK:       []string{contractMetadataRecord.Key().String()},
 		},
 		{
 			name: "Match existing address with labels",
@@ -275,6 +284,115 @@ func TestMemoryDataStore_Merge(t *testing.T) {
 
 			_, err = dataStore1.EnvMetadata().Get()
 			require.NoError(t, err, "Fetching env metadata from dataStore1 should not fail")
+
+			// Verify staged deletes were propagated to DeletedRemoteKeys
+			if len(tt.expectedAddressDRK) > 0 {
+				require.ElementsMatch(t, tt.expectedAddressDRK, dataStore1.AddressRefStore.DeletedRemoteKeys,
+					"dataStore1 AddressRefStore.DeletedRemoteKeys should contain the expected staged keys after merge")
+			}
+			if len(tt.expectedChainMetaDRK) > 0 {
+				require.ElementsMatch(t, tt.expectedChainMetaDRK, dataStore1.ChainMetadataStore.DeletedRemoteKeys,
+					"dataStore1 ChainMetadataStore.DeletedRemoteKeys should contain the expected staged keys after merge")
+			}
+			if len(tt.expectedContractMetaDRK) > 0 {
+				require.ElementsMatch(t, tt.expectedContractMetaDRK, dataStore1.ContractMetadataStore.DeletedRemoteKeys,
+					"dataStore1 ContractMetadataStore.DeletedRemoteKeys should contain the expected staged keys after merge")
+			}
 		})
 	}
 }
+
+func TestMemoryDataStore_Merge_ChainedComposition(t *testing.T) {
+	t.Parallel()
+
+	recA := AddressRef{
+		Address:   "0xAAA",
+		Type:      "typeA",
+		Version:   semver.MustParse("2.0.0"),
+		Qualifier: "qa",
+	}
+
+	t.Run("Chained merge preserves DRK across two hops", func(t *testing.T) {
+		t.Parallel()
+
+		dataStore1 := NewMemoryDataStore()
+		require.NoError(t, dataStore1.Addresses().Add(recA))
+
+		dataStore2 := NewMemoryDataStore()
+		require.NoError(t, dataStore2.Addresses().RemoteDelete(recA.Key()))
+
+		dataStore3 := NewMemoryDataStore()
+		require.NoError(t, dataStore3.Merge(dataStore2.Seal()))
+
+		dataStore4 := NewMemoryDataStore()
+		require.NoError(t, dataStore4.Merge(dataStore3.Seal()))
+
+		require.Contains(t, dataStore4.AddressRefStore.DeletedRemoteKeys, recA.Key().String(),
+			"DRK should survive two merge hops")
+
+		addressRefs, err := dataStore4.Addresses().Fetch()
+		require.NoError(t, err)
+		require.Empty(t, addressRefs, "recA should not appear in dataStore4 records after chained deletes")
+	})
+
+	t.Run("Merge is idempotent on staged deletes", func(t *testing.T) {
+		t.Parallel()
+
+		dataStore1 := NewMemoryDataStore()
+		require.NoError(t, dataStore1.Addresses().Add(recA))
+
+		dataStore2 := NewMemoryDataStore()
+		require.NoError(t, dataStore2.Addresses().RemoteDelete(recA.Key()))
+
+		sealed := dataStore2.Seal()
+
+		require.NoError(t, dataStore1.Merge(sealed), "first merge should succeed")
+		require.NoError(t, dataStore1.Merge(sealed), "second merge should succeed (idempotent)")
+
+		addressRefs, err := dataStore1.Addresses().Fetch()
+		require.NoError(t, err)
+		require.Empty(t, addressRefs, "recA should not be present after idempotent merges")
+
+		require.Contains(t, dataStore1.AddressRefStore.DeletedRemoteKeys, recA.Key().String(),
+			"DRK should be present after idempotent merges")
+	})
+}
+
+// TestMemoryDataStore_Merge_SourceLiveRecordClearsDestStagedDelete pins the
+// precedence rule documented on MemoryDataStore.Merge: when src.<Store>.Records
+// contains a live record for key K and src.<Store>.DeletedRemoteKeys does NOT
+// contain K, Merge's upsert clears K from dst.<Store>.DeletedRemoteKeys.
+//
+// In other words, a live record on the source side overrides a staged delete on
+// the destination side for the same key. Staged deletes are sticky only on the
+// source side of Merge.
+//
+// If this test starts failing, the precedence note on MemoryDataStore.Merge is
+// out of date — update either the assertion or the docstring to keep them in sync.
+func TestMemoryDataStore_Merge_SourceLiveRecordClearsDestStagedDelete(t *testing.T) {
+	t.Parallel()
+
+	rec := AddressRef{
+		Address:   "0xBEEF",
+		Type:      "typeP",
+		Version:   semver.MustParse("1.0.0"),
+		Qualifier: "qP",
+	}
+
+	dst := NewMemoryDataStore()
+	require.NoError(t, dst.Addresses().RemoteDelete(rec.Key()))
+	require.Contains(t, dst.AddressRefStore.DeletedRemoteKeys, rec.Key().String(),
+		"precondition: dst.DRK should contain the staged key before Merge")
+
+	src := NewMemoryDataStore()
+	require.NoError(t, src.Addresses().Add(rec))
+
+	require.NoError(t, dst.Merge(src.Seal()))
+
+	addressRefs, err := dst.Addresses().Fetch()
+	require.NoError(t, err)
+	require.Len(t, addressRefs, 1, "dst should now contain src's live record")
+
+	require.NotContains(t, dst.AddressRefStore.DeletedRemoteKeys, rec.Key().String(),
+		"dst.DRK should no longer contain the previously-staged key — Upsert cleared it")
+}