From 9f35a8028576826cf2ec154049bbe7535894f52d Mon Sep 17 00:00:00 2001 From: giogam <151543+giogam@users.noreply.github.com> Date: Thu, 11 Jun 2026 17:35:37 +0200 Subject: [PATCH 1/3] fix(datastore): preserve staged deletes across Merge composition MemoryDataStore.Merge propagated src.DeletedRemoteKeys by calling dst..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..RemoteDelete(key) followed by dst..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 ErrNotFound 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. --- datastore/memory_datastore.go | 38 +++++++- datastore/memory_datastore_test.go | 136 +++++++++++++++++++++++++++-- 2 files changed, 161 insertions(+), 13 deletions(-) diff --git a/datastore/memory_datastore.go b/datastore/memory_datastore.go index df2867d2e..cb680b936 100644 --- a/datastore/memory_datastore.go +++ b/datastore/memory_datastore.go @@ -62,7 +62,28 @@ 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 — persistence backends (JSON file, catalog) drive their +// remote deletes off DeletedRemoteKeys, so a Merge that only removed records and left +// DeletedRemoteKeys empty would silently drop the delete on every hop. +// +// 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 +105,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 +133,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 +162,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 f8a26600a..44b1cafcf 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") +} From e6bfc3daaa9dbf132af0262ed2d8203946222f5f Mon Sep 17 00:00:00 2001 From: Giorgio Gambino <151543+giogam@users.noreply.github.com> Date: Thu, 11 Jun 2026 17:44:41 +0200 Subject: [PATCH 2/3] Preserve staged deletes across merge composition Fixes the datastore to ensure that staged deletes are preserved during merge composition. --- .changeset/large-wombats-fall.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/large-wombats-fall.md diff --git a/.changeset/large-wombats-fall.md b/.changeset/large-wombats-fall.md new file mode 100644 index 000000000..693f06267 --- /dev/null +++ b/.changeset/large-wombats-fall.md @@ -0,0 +1,5 @@ +--- +"chainlink-deployments-framework": patch +--- + +fix(datastore): preserve staged deletes across Merge composition From 0e33953022a46ed919667cc6c4153e0ec6f771ec Mon Sep 17 00:00:00 2001 From: giogam <151543+giogam@users.noreply.github.com> Date: Fri, 12 Jun 2026 12:58:30 +0200 Subject: [PATCH 3/3] chore: minor docstring update --- datastore/memory_datastore.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/datastore/memory_datastore.go b/datastore/memory_datastore.go index cb680b936..ebea4d075 100644 --- a/datastore/memory_datastore.go +++ b/datastore/memory_datastore.go @@ -68,9 +68,7 @@ func (s *MemoryDataStore) WriteMetadata(bundle MetadataBundle, opts ...WriteMeta // 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 — persistence backends (JSON file, catalog) drive their -// remote deletes off DeletedRemoteKeys, so a Merge that only removed records and left -// DeletedRemoteKeys empty would silently drop the delete on every hop. +// 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