diff --git a/.changeset/large-wombats-fall.md b/.changeset/large-wombats-fall.md new file mode 100644 index 00000000..693f0626 --- /dev/null +++ b/.changeset/large-wombats-fall.md @@ -0,0 +1,5 @@ +--- +"chainlink-deployments-framework": patch +--- + +fix(datastore): preserve staged deletes across Merge composition diff --git a/datastore/memory_datastore.go b/datastore/memory_datastore.go index df2867d2..ebea4d07 100644 --- a/datastore/memory_datastore.go +++ b/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..DeletedRemoteKeys) are propagated by appending the +// key to s..DeletedRemoteKeys via RemoteDelete and then removing the record +// from s..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..Records overrides a staged delete in +// s..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 } } diff --git a/datastore/memory_datastore_test.go b/datastore/memory_datastore_test.go index f8a26600..44b1cafc 100644 --- a/datastore/memory_datastore_test.go +++ b/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..Records +// contains a live record for key K and src..DeletedRemoteKeys does NOT +// contain K, Merge's upsert clears K from dst..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") +}