From e603721322ba688a3ac2c64278aa27f6d1afe7da Mon Sep 17 00:00:00 2001 From: guillaumemichel Date: Wed, 15 Apr 2026 13:41:04 +0200 Subject: [PATCH 01/12] feat: add onPut/onDelete hooks with correct tombstone hook semantics --- crdt.go | 67 ++++--- crdt_test.go | 213 ++++++++++++++++++++--- set.go | 133 ++++++++++++-- set_test.go | 479 +++++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 820 insertions(+), 72 deletions(-) create mode 100644 set_test.go diff --git a/crdt.go b/crdt.go index 595d8e7..48e82ed 100644 --- a/crdt.go +++ b/crdt.go @@ -85,19 +85,31 @@ type Options struct { // been already broadcasted by other replicas in the // interval. Default: 1m. RebroadcastInterval time.Duration - // The PutHook function is triggered whenever an element - // is successfully added to the datastore (either by a local - // or remote update), and only when that addition is considered the - // prevalent value. Default: nil. + // PutHook is triggered whenever an element is successfully added to the + // datastore (either by a local or remote update), and only when that + // addition is considered the prevalent value. Default: nil. + // Mutually exclusive with OnPut. PutHook func(k ds.Key, v []byte) - // The DeleteHook function is triggered whenever a version of an - // element is successfully removed from the datastore (either by a - // local or remote update). Unordered and concurrent updates may - // result in the DeleteHook being triggered even though the element is - // still present in the datastore because it was re-added or not fully - // tombstoned. If that is relevant, use Has() to check if the removed - // element is still part of the datastore. Default: nil. + // OnPut is like PutHook but also receives the previous value for the key + // (nil if the key did not exist before). This incurs an extra datastore + // read per put. Mutually exclusive with PutHook. Default: nil. + // The callback is invoked while internal locks are held; it must not + // call back into the Datastore or it will deadlock. + OnPut func(k ds.Key, newVal, oldVal []byte) + // DeleteHook is triggered whenever a version of an element is + // successfully removed from the datastore (either by a local or remote + // update). Unordered and concurrent updates may result in the DeleteHook + // being triggered even though the element is still present in the + // datastore because it was re-added or not fully tombstoned. If that is + // relevant, use Has() to check if the removed element is still part of + // the datastore. Default: nil. Mutually exclusive with OnDelete. DeleteHook func(k ds.Key) + // OnDelete is like DeleteHook but also receives the last known value for + // the key before it was removed. This incurs an extra datastore read per + // delete. Mutually exclusive with DeleteHook. Default: nil. + // The callback is invoked while internal locks are held; it must not + // call back into the Datastore or it will deadlock. + OnDelete func(k ds.Key, lastVal []byte) // NumWorkers specifies the number of workers ready to // retrieve and merge deltas while walking the DAGs. Default: // 5. @@ -186,7 +198,9 @@ func DefaultOptions() *Options { Logger: logging.Logger("crdt"), RebroadcastInterval: time.Minute, PutHook: nil, + OnPut: nil, DeleteHook: nil, + OnDelete: nil, NumWorkers: 5, DAGSyncerTimeout: 5 * time.Minute, // always keeping @@ -292,6 +306,19 @@ func New( return nil, err } + if opts.PutHook != nil && opts.OnPut != nil { + return nil, errors.New("PutHook and OnPut are mutually exclusive") + } + if opts.DeleteHook != nil && opts.OnDelete != nil { + return nil, errors.New("DeleteHook and OnDelete are mutually exclusive") + } + hooks := setHooks{ + putHook: opts.PutHook, + onPut: opts.OnPut, + deleteHook: opts.DeleteHook, + onDelete: opts.OnDelete, + } + // /set fullSetNs := namespace.ChildString(opts.crdtOpts.Namespaces.Set) // /heads @@ -300,24 +327,8 @@ func New( // /heads fullDagHeadsNs := namespace.ChildString(opts.crdtOpts.Namespaces.DAGHeads) - setPutHook := func(k string, v []byte) { - if opts.PutHook == nil { - return - } - dsk := ds.NewKey(k) - opts.PutHook(dsk, v) - } - - setDeleteHook := func(k string) { - if opts.DeleteHook == nil { - return - } - dsk := ds.NewKey(k) - opts.DeleteHook(dsk) - } - ctx, cancel := context.WithCancel(context.Background()) - set, err := newCRDTSet(ctx, store, fullSetNs, dagSyncer, opts.Logger, setPutHook, setDeleteHook, opts.crdtOpts.DeltaFactory) + set, err := newCRDTSet(ctx, store, fullSetNs, dagSyncer, opts.Logger, hooks, opts.crdtOpts.DeltaFactory) if err != nil { cancel() return nil, fmt.Errorf("error setting up crdt set: %w", err) diff --git a/crdt_test.go b/crdt_test.go index 32b96aa..7042f82 100644 --- a/crdt_test.go +++ b/crdt_test.go @@ -9,6 +9,7 @@ import ( "sync" "sync/atomic" "testing" + "testing/synctest" "time" blockstore "github.com/ipfs/boxo/blockstore" @@ -566,39 +567,195 @@ func TestCRDTPrintDAG(t *testing.T) { } func TestCRDTHooks(t *testing.T) { - ctx := context.Background() + synctest.Test(t, func(t *testing.T) { + ctx := t.Context() + var put int64 + var deleted int64 + + opts := DefaultOptions() + opts.PutHook = func(k ds.Key, v []byte) { + atomic.AddInt64(&put, 1) + } + opts.DeleteHook = func(k ds.Key) { + atomic.AddInt64(&deleted, 1) + } - var put int64 - var deleted int64 + replicas, closeReplicas := makeReplicas(t, opts) + t.Cleanup(closeReplicas) - opts := DefaultOptions() - opts.PutHook = func(k ds.Key, v []byte) { - atomic.AddInt64(&put, 1) - } - opts.DeleteHook = func(k ds.Key) { - atomic.AddInt64(&deleted, 1) - } + k := ds.RandomKey() + if err := replicas[0].Put(ctx, k, nil); err != nil { + t.Fatal(err) + } + if err := replicas[0].Delete(ctx, k); err != nil { + t.Fatal(err) + } + synctest.Wait() + if atomic.LoadInt64(&put) != int64(len(replicas)) { + t.Error("all replicas should have notified Put", put) + } + if atomic.LoadInt64(&deleted) != int64(len(replicas)) { + t.Error("all replicas should have notified Remove", deleted) + } + }) +} - replicas, closeReplicas := makeReplicas(t, opts) - defer closeReplicas() +func TestCRDTOnPut(t *testing.T) { + t.Parallel() + synctest.Test(t, func(t *testing.T) { + type hookCall struct { + key ds.Key + newVal []byte + oldVal []byte + } + var calls []hookCall + var mu sync.Mutex + + opts := DefaultOptions() + opts.OnPut = func(k ds.Key, newVal, oldVal []byte) { + mu.Lock() + calls = append(calls, hookCall{k, newVal, oldVal}) + mu.Unlock() + } - k := ds.RandomKey() - err := replicas[0].Put(ctx, k, nil) - if err != nil { - t.Fatal(err) - } + replicas, closeReplicas := makeReplicas(t, opts) + t.Cleanup(closeReplicas) - err = replicas[0].Delete(ctx, k) - if err != nil { - t.Fatal(err) - } - time.Sleep(100 * time.Millisecond) - if atomic.LoadInt64(&put) != int64(len(replicas)) { - t.Error("all replicas should have notified Put", put) - } - if atomic.LoadInt64(&deleted) != int64(len(replicas)) { - t.Error("all replicas should have notified Remove", deleted) - } + k := ds.NewKey("/testkey") + ctx := t.Context() + + // First put: all replicas should fire onPut with newVal="first", oldVal=nil. + if err := replicas[0].Put(ctx, k, []byte("first")); err != nil { + t.Fatal(err) + } + synctest.Wait() + + mu.Lock() + if len(calls) != len(replicas) { + t.Errorf("expected %d OnPut calls (one per replica), got %d", len(replicas), len(calls)) + } + for i, c := range calls { + if string(c.newVal) != "first" { + t.Errorf("call[%d]: expected newVal %q, got %q", i, "first", c.newVal) + } + if c.oldVal != nil { + t.Errorf("call[%d]: expected oldVal nil on first put, got %q", i, c.oldVal) + } + } + calls = nil + mu.Unlock() + + // Second put: all replicas should fire onPut with newVal="second", oldVal="first". + if err := replicas[0].Put(ctx, k, []byte("second")); err != nil { + t.Fatal(err) + } + synctest.Wait() + + mu.Lock() + defer mu.Unlock() + if len(calls) != len(replicas) { + t.Errorf("expected %d OnPut calls on second put, got %d", len(replicas), len(calls)) + } + for i, c := range calls { + if string(c.newVal) != "second" { + t.Errorf("call[%d]: expected newVal %q, got %q", i, "second", c.newVal) + } + if string(c.oldVal) != "first" { + t.Errorf("call[%d]: expected oldVal %q, got %q", i, "first", c.oldVal) + } + } + }) +} + +func TestCRDTOnDelete(t *testing.T) { + t.Parallel() + synctest.Test(t, func(t *testing.T) { + type hookCall struct { + key ds.Key + lastVal []byte + } + var calls []hookCall + var mu sync.Mutex + + opts := DefaultOptions() + opts.OnDelete = func(k ds.Key, lastVal []byte) { + mu.Lock() + calls = append(calls, hookCall{k, lastVal}) + mu.Unlock() + } + + replicas, closeReplicas := makeReplicas(t, opts) + t.Cleanup(closeReplicas) + + ctx := t.Context() + k := ds.NewKey("/testkey") + if err := replicas[0].Put(ctx, k, []byte("hello")); err != nil { + t.Fatal(err) + } + synctest.Wait() + + if err := replicas[0].Delete(ctx, k); err != nil { + t.Fatal(err) + } + synctest.Wait() + + mu.Lock() + if len(calls) != len(replicas) { + t.Errorf("expected %d OnDelete calls (one per replica), got %d", len(replicas), len(calls)) + } + for i, c := range calls { + if string(c.lastVal) != "hello" { + t.Errorf("call[%d]: expected lastVal %q, got %q", i, "hello", c.lastVal) + } + } + calls = nil + mu.Unlock() + + // Delete the same key again — it no longer exists. Rmv produces no + // tombstones so Delete returns early without publishing — the hook + // must not fire. + if err := replicas[0].Delete(ctx, k); err != nil { + t.Fatal(err) + } + synctest.Wait() + + mu.Lock() + defer mu.Unlock() + if n := len(calls); n != 0 { + t.Errorf("OnDelete should not be called for a non-existent key, got %d calls", n) + } + }) +} + +func TestCRDTHooksMutuallyExclusive(t *testing.T) { + t.Parallel() + store := dssync.MutexWrap(ds.NewMapDatastore()) + dagService := mdutils.Mock() + namespace := ds.NewKey("/test") + + t.Run("PutHook and OnPut", func(t *testing.T) { + bcasts, cancelBcasts := newBroadcasters(t, 1) + t.Cleanup(cancelBcasts) + opts := DefaultOptions() + opts.PutHook = func(ds.Key, []byte) {} + opts.OnPut = func(ds.Key, []byte, []byte) {} + _, err := New(store, namespace, dagService, bcasts[0], opts) + if err == nil { + t.Fatal("expected error when both PutHook and OnPut are set") + } + }) + + t.Run("DeleteHook and OnDelete", func(t *testing.T) { + bcasts, cancelBcasts := newBroadcasters(t, 1) + t.Cleanup(cancelBcasts) + opts := DefaultOptions() + opts.DeleteHook = func(ds.Key) {} + opts.OnDelete = func(ds.Key, []byte) {} + _, err := New(store, namespace, dagService, bcasts[0], opts) + if err == nil { + t.Fatal("expected error when both DeleteHook and OnDelete are set") + } + }) } func TestCRDTBatch(t *testing.T) { diff --git a/set.go b/set.go index fbe5b10..798867e 100644 --- a/set.go +++ b/set.go @@ -36,6 +36,15 @@ type Set interface { InSet(ctx context.Context, key string) (bool, error) } +// setHooks holds the hook functions for put and delete events. Only one of +// putHook or onPut may be set; likewise for deleteHook and onDelete. +type setHooks struct { + putHook func(key ds.Key, v []byte) + onPut func(key ds.Key, newVal, oldVal []byte) + deleteHook func(key ds.Key) + onDelete func(key ds.Key, lastVal []byte) +} + // set implements an Add-Wins Observed-Remove Set using delta-CRDTs // (https://arxiv.org/abs/1410.2803) and backing all the data in a // go-datastore. It is fully agnostic to MerkleCRDTs or the delta distribution @@ -46,8 +55,7 @@ type set struct { store ds.Datastore dagService ipld.DAGService namespace ds.Key - putHook func(key string, v []byte) - deleteHook func(key string) + hooks setHooks deltaFactory func() Delta logger logging.StandardLogger @@ -62,8 +70,7 @@ func newCRDTSet( namespace ds.Key, dagService ipld.DAGService, logger logging.StandardLogger, - putHook func(key string, v []byte), - deleteHook func(key string), + hooks setHooks, deltaFactory func() Delta, ) (*set, error) { set := &set{ @@ -71,14 +78,37 @@ func newCRDTSet( store: d, dagService: dagService, logger: logger, - putHook: putHook, - deleteHook: deleteHook, + hooks: hooks, deltaFactory: deltaFactory, } return set, nil } +// triggerPutHook calls the appropriate put hook with newVal and oldVal. +// oldVal must have been fetched from the store before the write. +func (s *set) triggerPutHook(key string, newVal, oldVal []byte) { + if s.hooks.onPut != nil { + s.hooks.onPut(ds.NewKey(key), newVal, oldVal) + return + } + if s.hooks.putHook != nil { + s.hooks.putHook(ds.NewKey(key), newVal) + } +} + +// triggerDeleteHook calls the appropriate delete hook with lastVal, +// the value that was stored before the deletion. +func (s *set) triggerDeleteHook(key string, lastVal []byte) { + if s.hooks.onDelete != nil { + s.hooks.onDelete(ds.NewKey(key), lastVal) + return + } + if s.hooks.deleteHook != nil { + s.hooks.deleteHook(ds.NewKey(key)) + } +} + // Add returns a new delta-set adding the given key/value. func (s *set) Add(ctx context.Context, key string, value []byte) (Delta, error) { delta := s.deltaFactory() @@ -357,12 +387,22 @@ func (s *set) setValue(ctx context.Context, writeStore ds.Write, key, id string, } valueK := s.valueKey(key) + var oldVal []byte if prio == curPrio { - curValue, _ := s.store.Get(ctx, valueK) + oldVal, err = s.store.Get(ctx, valueK) + if err != nil && !errors.Is(err, ds.ErrNotFound) { + s.logger.Errorf("error reading current value for key %s: %s", key, err) + } // new value greater than old - if bytes.Compare(curValue, value) >= 0 { + if bytes.Compare(oldVal, value) >= 0 { return nil } + } else if s.hooks.onPut != nil { + // Fetch old value before any write for the onPut hook. + oldVal, err = s.store.Get(ctx, valueK) + if err != nil && !errors.Is(err, ds.ErrNotFound) { + s.logger.Errorf("error reading current value for onPut hook: key %s: %s", key, err) + } } // store value @@ -378,7 +418,7 @@ func (s *set) setValue(ctx context.Context, writeStore ds.Write, key, id string, } // trigger add hook - s.putHook(key, value) + s.triggerPutHook(key, value, oldVal) return nil } @@ -567,12 +607,43 @@ func (s *set) putTombs(ctx context.Context, tombs []*pb.Element) error { // key -> tombstonedBlockID. Carries the tombstoned blocks for each // element in this delta. deletedElems := make(map[string][]string) + // newVals holds the winning value for keys that were partially tombstoned + // (tombstone removed a previous winner but a surviving element took over). + // A key absent from this map was fully deleted. Doubles as the + // fully-deleted oracle, so it is allocated whenever any hook is registered; + // nil means no hooks are configured and the firing loop is skipped entirely. + var newVals map[string][]byte + if s.hooks.putHook != nil || s.hooks.onPut != nil || s.hooks.deleteHook != nil || s.hooks.onDelete != nil { + newVals = make(map[string][]byte) + } + // prevVals caches the value at the time each key is first seen in this + // delta, before any write. Only keys that existed in the store are added; + // absent keys are omitted so the two-value map lookup (v, ok) cleanly + // distinguishes "had a value" from "was not in the store". + var prevVals map[string][]byte + if newVals != nil { + prevVals = make(map[string][]byte) + } var errs []error for _, e := range tombs { // /namespace/tombs// key := e.GetKey() id := e.GetId() valueK := s.valueKey(key) + + // Capture the current value on first encounter, before any write for + // this key, so hooks receive the pre-tombstone value. + if prevVals != nil { + if _, seen := prevVals[key]; !seen { + curVal, err := s.store.Get(ctx, valueK) + if err == nil { + prevVals[key] = curVal + } else if !errors.Is(err, ds.ErrNotFound) { + s.logger.Errorf("error reading value before tombstone for key %s: %s", key, err) + } + // ErrNotFound: omit from map; hook firing uses (v, ok) to detect absence. + } + } deletedElems[key] = append(deletedElems[key], id) // Find best value for element that we are going to delete @@ -582,6 +653,7 @@ func (s *set) putTombs(ctx context.Context, tombs []*pb.Element) error { } if v == nil { + delete(newVals, key) if err = store.Delete(ctx, valueK); err != nil { errs = append(errs, err) } @@ -589,6 +661,9 @@ func (s *set) putTombs(ctx context.Context, tombs []*pb.Element) error { errs = append(errs, err) } } else { + if newVals != nil { + newVals[key] = v + } if err = store.Put(ctx, valueK, v); err != nil { errs = append(errs, err) } @@ -614,11 +689,26 @@ func (s *set) putTombs(ctx context.Context, tombs []*pb.Element) error { } } - // run delete hook only once for all versions of the same element - // tombstoned in this delta. Note it may be that the element was not - // fully deleted and only a different value took its place. - for del := range deletedElems { - s.deleteHook(del) + // Fire hooks once per key after all writes are committed. + // Skipped entirely when no hooks are registered (newVals == nil). + // Fully deleted keys (absent from newVals) trigger the delete hook; + // partially tombstoned keys (present in newVals) trigger the put hook. + if newVals != nil { + for del := range deletedElems { + if newVal, partial := newVals[del]; partial { + oldVal := prevVals[del] // nil if key was absent + if bytes.Equal(newVal, oldVal) { + continue // value unchanged, skip hook + } + s.triggerPutHook(del, newVal, oldVal) + } else { + lastVal, ok := prevVals[del] + if !ok { + continue // key had no value before tombstone, nothing to notify + } + s.triggerDeleteHook(del, lastVal) + } + } } return nil @@ -764,6 +854,17 @@ func (s *set) purgeKeyBlocks(ctx context.Context, key string, blockCIDs map[cid. } valueK := s.valueKey(key) + + // Fetch old value before modifying the value key, so hooks receive the + // pre-purge value. Only read when a hook that needs it is configured. + var oldVal []byte + if s.hooks.onPut != nil || s.hooks.onDelete != nil { + oldVal, err = s.store.Get(ctx, valueK) + if err != nil && !errors.Is(err, ds.ErrNotFound) { + s.logger.Errorf("error reading value before purge for hook: key %s: %s", key, err) + } + } + if bestVal == nil { var errs []error if err := s.store.Delete(ctx, valueK); err != nil && !errors.Is(err, ds.ErrNotFound) { @@ -775,7 +876,7 @@ func (s *set) purgeKeyBlocks(ctx context.Context, key string, blockCIDs map[cid. if err := errors.Join(errs...); err != nil { return err } - s.deleteHook(key) + s.triggerDeleteHook(key, oldVal) } else { if err := s.store.Put(ctx, valueK, bestVal); err != nil { return err @@ -783,7 +884,7 @@ func (s *set) purgeKeyBlocks(ctx context.Context, key string, blockCIDs map[cid. if err := s.setPriority(ctx, s.store, key, bestPrio); err != nil { return err } - s.putHook(key, bestVal) + s.triggerPutHook(key, bestVal, oldVal) } return nil diff --git a/set_test.go b/set_test.go new file mode 100644 index 0000000..33ddde0 --- /dev/null +++ b/set_test.go @@ -0,0 +1,479 @@ +package crdt + +import ( + "testing" + + dshelp "github.com/ipfs/boxo/datastore/dshelp" + mdutils "github.com/ipfs/boxo/ipld/merkledag/test" + ds "github.com/ipfs/go-datastore" + dssync "github.com/ipfs/go-datastore/sync" + pb "github.com/ipfs/go-ds-crdt/pb" + ipld "github.com/ipfs/go-ipld-format" +) + +func newTestSet(t *testing.T, dagService ipld.DAGService, hooks setHooks) *set { + t.Helper() + store := dssync.MutexWrap(ds.NewMapDatastore()) + df := func() Delta { return &pbDelta{Delta: &pb.Delta{}} } + s, err := newCRDTSet(t.Context(), store, ds.NewKey("/testset"), dagService, DefaultOptions().Logger, hooks, df) + if err != nil { + t.Fatalf("newCRDTSet: %v", err) + } + return s +} + +// addElem creates a real DAG block for (key, value, prio), stores it in +// dagService, seeds the set with putElems, and returns the block key (element ID). +func addElem(t *testing.T, s *set, dagService ipld.DAGService, key string, value []byte, prio uint64) string { + t.Helper() + ctx := t.Context() + d := &pbDelta{Delta: &pb.Delta{}} + d.SetElements([]*pb.Element{{Key: key, Value: value}}) + d.SetPriority(prio) + + node, err := makeNode(d, nil) + if err != nil { + t.Fatalf("makeNode: %v", err) + } + if err := dagService.Add(ctx, node); err != nil { + t.Fatalf("dagService.Add: %v", err) + } + blockKey := dshelp.MultihashToDsKey(node.Cid().Hash()).String() + + elems, err := d.GetElements() + if err != nil { + t.Fatalf("GetElements: %v", err) + } + if err := s.putElems(ctx, elems, blockKey, prio); err != nil { + t.Fatalf("putElems: %v", err) + } + return blockKey +} + +// TestPutTombsEmpty verifies that putTombs with an empty list is a no-op. +func TestPutTombsEmpty(t *testing.T) { + t.Parallel() + ctx := t.Context() + var fired bool + s := newTestSet(t, mdutils.Mock(), setHooks{ + putHook: func(ds.Key, []byte) { fired = true }, + deleteHook: func(ds.Key) { fired = true }, + }) + + if err := s.putTombs(ctx, nil); err != nil { + t.Fatal(err) + } + if err := s.putTombs(ctx, []*pb.Element{}); err != nil { + t.Fatal(err) + } + if fired { + t.Error("no hooks should fire for an empty tombstone list") + } +} + +// TestPutTombsFullDelete verifies tombstoning the only element for a key: +// the store is cleaned up and the appropriate hook fires. +func TestPutTombsFullDelete(t *testing.T) { + t.Parallel() + + t.Run("store cleanup", func(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + s := newTestSet(t, dag, setHooks{}) + id := addElem(t, s, dag, "foo", []byte("hello"), 1) + + if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id}}); err != nil { + t.Fatal(err) + } + if _, err := s.store.Get(ctx, s.valueKey("foo")); err != ds.ErrNotFound { + t.Errorf("value key should be deleted, got err=%v", err) + } + if _, err := s.store.Get(ctx, s.priorityKey("foo")); err != ds.ErrNotFound { + t.Errorf("priority key should be deleted, got err=%v", err) + } + if inSet, _ := s.InSet(ctx, "foo"); inSet { + t.Error("key should not be in set after full tombstone") + } + }) + + t.Run("deleteHook", func(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + var keys []ds.Key + s := newTestSet(t, dag, setHooks{deleteHook: func(k ds.Key) { keys = append(keys, k) }}) + id := addElem(t, s, dag, "foo", []byte("hello"), 1) + + if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id}}); err != nil { + t.Fatal(err) + } + if len(keys) != 1 || keys[0].String() != ds.NewKey("foo").String() { + t.Errorf("deleteHook calls = %v, want [/foo]", keys) + } + }) + + t.Run("onDelete receives lastVal", func(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + var gotKey ds.Key + var gotVal []byte + s := newTestSet(t, dag, setHooks{ + onDelete: func(k ds.Key, v []byte) { gotKey, gotVal = k, v }, + }) + id := addElem(t, s, dag, "foo", []byte("hello"), 1) + + if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id}}); err != nil { + t.Fatal(err) + } + if gotKey.String() != ds.NewKey("foo").String() { + t.Errorf("onDelete key = %q, want /foo", gotKey) + } + if string(gotVal) != "hello" { + t.Errorf("onDelete lastVal = %q, want hello", gotVal) + } + }) +} + +// TestPutTombsNilValue verifies correct hook behavior when a key's stored value +// is nil (e.g. Put(ctx, k, nil)). A nil stored value is distinct from "key not +// found": delete hooks must fire, and onDelete must receive nil as lastVal. +func TestPutTombsNilValue(t *testing.T) { + t.Parallel() + + t.Run("deleteHook fires for nil value", func(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + var deleteFired bool + s := newTestSet(t, dag, setHooks{ + deleteHook: func(ds.Key) { deleteFired = true }, + }) + id := addElem(t, s, dag, "foo", nil, 1) + + if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id}}); err != nil { + t.Fatal(err) + } + if !deleteFired { + t.Error("deleteHook must fire even when the stored value is nil") + } + }) + + t.Run("onDelete receives nil lastVal", func(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + var gotKey ds.Key + var gotVal []byte + var called bool + s := newTestSet(t, dag, setHooks{ + onDelete: func(k ds.Key, v []byte) { gotKey, gotVal, called = k, v, true }, + }) + id := addElem(t, s, dag, "foo", nil, 1) + + if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id}}); err != nil { + t.Fatal(err) + } + if !called { + t.Fatal("onDelete must be called") + } + if gotKey.String() != ds.NewKey("foo").String() { + t.Errorf("onDelete key = %q, want /foo", gotKey) + } + if gotVal != nil { + t.Errorf("onDelete lastVal = %q, want nil", gotVal) + } + }) + + t.Run("nil survivor treated as full delete", func(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + var deleteFired bool + var deleteLastVal []byte + var putFired bool + s := newTestSet(t, dag, setHooks{ + onPut: func(ds.Key, []byte, []byte) { putFired = true }, + onDelete: func(_ ds.Key, v []byte) { deleteFired = true; deleteLastVal = v }, + }) + // "aaa" wins over nil lexicographically; store value is "aaa". + id1 := addElem(t, s, dag, "foo", nil, 1) // loses + id2 := addElem(t, s, dag, "foo", []byte("aaa"), 1) // wins + putFired = false + + // NOTE: this test documents a known limitation. After tombstoning id2, + // id1 (nil value) is still a live, non-tombstoned element — so the key + // should logically remain in the set with a nil value and fire onPut. + // However, findBestValue returns nil for both "no survivors" and + // "one survivor whose value is nil", so putTombs cannot distinguish the + // two cases. It takes the full-delete path, removes the value key from + // the store, and fires onDelete. As a result InSet returns false even + // though a surviving element exists. Fixing this requires findBestValue + // to return a separate found bool alongside the value. + if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id2}}); err != nil { + t.Fatal(err) + } + _ = id1 + if putFired { + t.Error("onPut must not fire; nil survivor is treated as full delete") + } + if !deleteFired { + t.Fatal("onDelete must fire when nil survivor is treated as full delete") + } + if string(deleteLastVal) != "aaa" { + t.Errorf("onDelete lastVal = %q, want aaa", deleteLastVal) + } + }) +} + +// TestPutTombsPartialTombstone verifies that tombstoning one of two elements +// leaves the survivor in the store and fires the right hook. +// Two elements at equal priority: "bbb" wins lexicographically over "aaa". +// Tombstoning the "aaa" block leaves "bbb" as the survivor. +func TestPutTombsPartialTombstone(t *testing.T) { + t.Parallel() + + t.Run("surviving value kept", func(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + s := newTestSet(t, dag, setHooks{}) + id1 := addElem(t, s, dag, "foo", []byte("aaa"), 1) + addElem(t, s, dag, "foo", []byte("bbb"), 1) + + if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id1}}); err != nil { + t.Fatal(err) + } + val, err := s.Element(ctx, "foo") + if err != nil { + t.Fatalf("Element: %v", err) + } + if string(val) != "bbb" { + t.Errorf("surviving value = %q, want bbb", val) + } + }) + + t.Run("no hook when value unchanged", func(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + var putFired, deleteFired bool + s := newTestSet(t, dag, setHooks{ + putHook: func(ds.Key, []byte) { putFired = true }, + deleteHook: func(ds.Key) { deleteFired = true }, + }) + id1 := addElem(t, s, dag, "foo", []byte("aaa"), 1) // "aaa" loses + addElem(t, s, dag, "foo", []byte("bbb"), 1) // "bbb" wins + putFired = false + + // Tombstone the loser: "bbb" remains the winner, no observable change. + if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id1}}); err != nil { + t.Fatal(err) + } + if putFired || deleteFired { + t.Error("no hook should fire when tombstoning a non-winning element") + } + }) + + t.Run("putHook fires not deleteHook", func(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + var putVals [][]byte + var deleteFired bool + s := newTestSet(t, dag, setHooks{ + putHook: func(_ ds.Key, v []byte) { putVals = append(putVals, v) }, + deleteHook: func(ds.Key) { deleteFired = true }, + }) + addElem(t, s, dag, "foo", []byte("aaa"), 1) + id2 := addElem(t, s, dag, "foo", []byte("bbb"), 1) // "bbb" wins, is current value + putVals = nil // discard calls from addElem + + // Tombstone the current winner ("bbb"). "aaa" becomes the new winner, + // so the value changes and putHook must fire. + if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id2}}); err != nil { + t.Fatal(err) + } + if deleteFired { + t.Error("deleteHook must not fire when a surviving element exists") + } + if len(putVals) != 1 || string(putVals[0]) != "aaa" { + t.Errorf("putHook vals = %q, want [aaa]", putVals) + } + }) + + t.Run("onPut receives newVal and oldVal", func(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + type call struct{ newVal, oldVal []byte } + var calls []call + s := newTestSet(t, dag, setHooks{ + onPut: func(_ ds.Key, n, o []byte) { calls = append(calls, call{n, o}) }, + }) + addElem(t, s, dag, "foo", []byte("aaa"), 1) + id2 := addElem(t, s, dag, "foo", []byte("bbb"), 1) // "bbb" wins, is current value + calls = nil + + // Tombstone the current winner ("bbb"). "aaa" becomes the new winner: + // onPut must fire with newVal="aaa", oldVal="bbb". + if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id2}}); err != nil { + t.Fatal(err) + } + if len(calls) != 1 { + t.Fatalf("expected 1 onPut call, got %d", len(calls)) + } + if string(calls[0].newVal) != "aaa" { + t.Errorf("onPut newVal = %q, want aaa", calls[0].newVal) + } + if string(calls[0].oldVal) != "bbb" { + t.Errorf("onPut oldVal = %q, want bbb", calls[0].oldVal) + } + }) +} + +// TestPutTombsMultipleKeys verifies that a single putTombs call handles +// tombstones for multiple keys independently. +func TestPutTombsMultipleKeys(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + deleted := make(map[string][]byte) + var onPutCalled bool + s := newTestSet(t, dag, setHooks{ + onDelete: func(k ds.Key, v []byte) { deleted[k.String()] = v }, + onPut: func(ds.Key, []byte, []byte) { onPutCalled = true }, + }) + id1 := addElem(t, s, dag, "key1", []byte("val1"), 1) + id2 := addElem(t, s, dag, "key2", []byte("val2"), 1) + onPutCalled = false // discard calls from addElem setup + + tombs := []*pb.Element{{Key: "key1", Id: id1}, {Key: "key2", Id: id2}} + if err := s.putTombs(ctx, tombs); err != nil { + t.Fatal(err) + } + if len(deleted) != 2 { + t.Fatalf("expected 2 onDelete calls, got %d", len(deleted)) + } + if string(deleted[ds.NewKey("key1").String()]) != "val1" { + t.Errorf("key1 lastVal = %q, want val1", deleted[ds.NewKey("key1").String()]) + } + if string(deleted[ds.NewKey("key2").String()]) != "val2" { + t.Errorf("key2 lastVal = %q, want val2", deleted[ds.NewKey("key2").String()]) + } + if onPutCalled { + t.Error("onPut must not fire for full-delete tombstones") + } +} + +// TestPutTombsNonExistentKey verifies that tombstoning a block ID for a key +// that was never PUT does not crash: the tomb is recorded, and no hooks fire +// since there was no prior value to report. +func TestPutTombsNonExistentKey(t *testing.T) { + t.Parallel() + ctx := t.Context() + var putFired, deleteFired bool + s := newTestSet(t, mdutils.Mock(), setHooks{ + onPut: func(ds.Key, []byte, []byte) { putFired = true }, + onDelete: func(ds.Key, []byte) { deleteFired = true }, + }) + + if err := s.putTombs(ctx, []*pb.Element{{Key: "ghost", Id: "fakeid"}}); err != nil { + t.Fatalf("putTombs: %v", err) + } + inTomb, err := s.inTombsKeyID(ctx, "ghost", "fakeid") + if err != nil { + t.Fatalf("inTombsKeyID: %v", err) + } + if !inTomb { + t.Error("tomb entry should be written even for a key never PUT") + } + if inSet, _ := s.InSet(ctx, "ghost"); inSet { + t.Error("key must not be in set") + } + if putFired { + t.Error("onPut must not fire for a key that was never PUT") + } + if deleteFired { + t.Error("onDelete must not fire when key had no prior value") + } +} + +// TestPutTombsPrevValsFirstEncounterOnly verifies that prevVals is captured +// before any write for a key: onDelete receives the pre-call value even when +// multiple tombs for the same key appear in one delta. +func TestPutTombsPrevValsFirstEncounterOnly(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + var putFired bool + var lastVals [][]byte + s := newTestSet(t, dag, setHooks{ + putHook: func(ds.Key, []byte) { putFired = true }, + onDelete: func(_ ds.Key, v []byte) { lastVals = append(lastVals, v) }, + }) + id1 := addElem(t, s, dag, "foo", []byte("first"), 1) + id2 := addElem(t, s, dag, "foo", []byte("second"), 2) // wins (higher prio) + putFired = false + + tombs := []*pb.Element{{Key: "foo", Id: id1}, {Key: "foo", Id: id2}} + if err := s.putTombs(ctx, tombs); err != nil { + t.Fatal(err) + } + if putFired { + t.Error("putHook must not fire when tombstoning a key with a surviving element") + } + if len(lastVals) != 1 { + t.Fatalf("expected 1 onDelete call, got %d", len(lastVals)) + } + if string(lastVals[0]) != "second" { + t.Errorf("onDelete lastVal = %q, want second", lastVals[0]) + } +} + +// TestPutTombsIdempotent verifies that applying the same tombstone twice +// leaves the key absent from the set. +func TestPutTombsIdempotent(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + var deleteFiredCount uint8 + s := newTestSet(t, dag, setHooks{deleteHook: func(ds.Key) { deleteFiredCount++ }}) + id := addElem(t, s, dag, "foo", []byte("hello"), 1) + tombs := []*pb.Element{{Key: "foo", Id: id}} + + for range 5 { + if err := s.putTombs(ctx, tombs); err != nil { + t.Fatal(err) + } + } + if inSet, _ := s.InSet(ctx, "foo"); inSet { + t.Error("key should not be in set after re-tombstoning") + } + if deleteFiredCount != 1 { + t.Errorf("deleteHook should fire only once for the same tombstone, got %d", deleteFiredCount) + } +} + +// TestPutTombsHigherPriorityWins verifies that the higher-priority element +// survives when the lower-priority one is tombstoned. +func TestPutTombsHigherPriorityWins(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + s := newTestSet(t, dag, setHooks{}) + id1 := addElem(t, s, dag, "foo", []byte("zzz"), 1) // high lex, low prio + addElem(t, s, dag, "foo", []byte("aaa"), 2) // low lex, high prio + + if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id1}}); err != nil { + t.Fatal(err) + } + val, err := s.Element(ctx, "foo") + if err != nil { + t.Fatalf("Element: %v", err) + } + if string(val) != "aaa" { + t.Errorf("surviving value = %q, want aaa (high-priority element)", val) + } +} From b4e00ad2d6466b1e26d221630822311a1240ce39 Mon Sep 17 00:00:00 2001 From: guillaumemichel Date: Fri, 17 Apr 2026 13:50:10 +0200 Subject: [PATCH 02/12] address review --- set.go | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/set.go b/set.go index 798867e..53b2a44 100644 --- a/set.go +++ b/set.go @@ -607,23 +607,23 @@ func (s *set) putTombs(ctx context.Context, tombs []*pb.Element) error { // key -> tombstonedBlockID. Carries the tombstoned blocks for each // element in this delta. deletedElems := make(map[string][]string) - // newVals holds the winning value for keys that were partially tombstoned - // (tombstone removed a previous winner but a surviving element took over). - // A key absent from this map was fully deleted. Doubles as the - // fully-deleted oracle, so it is allocated whenever any hook is registered; - // nil means no hooks are configured and the firing loop is skipped entirely. - var newVals map[string][]byte + + var newVals, prevVals map[string][]byte if s.hooks.putHook != nil || s.hooks.onPut != nil || s.hooks.deleteHook != nil || s.hooks.onDelete != nil { + // newVals holds the winning value for keys that were partially tombstoned + // (tombstone removed a previous winner but a surviving element took over). + // A key absent from this map was fully deleted. Doubles as the + // fully-deleted oracle, so it is allocated whenever any hook is registered; + // nil means no hooks are configured and the firing loop is skipped entirely. newVals = make(map[string][]byte) - } - // prevVals caches the value at the time each key is first seen in this - // delta, before any write. Only keys that existed in the store are added; - // absent keys are omitted so the two-value map lookup (v, ok) cleanly - // distinguishes "had a value" from "was not in the store". - var prevVals map[string][]byte - if newVals != nil { + + // prevVals caches the value at the time each key is first seen in this + // delta, before any write. Only keys that existed in the store are added; + // absent keys are omitted so the two-value map lookup (v, ok) cleanly + // distinguishes "had a value" from "was not in the store". prevVals = make(map[string][]byte) } + var errs []error for _, e := range tombs { // /namespace/tombs// From bf189e7a4c58a4d5d01dd1b4ea89810db780e1ae Mon Sep 17 00:00:00 2001 From: guillaumemichel Date: Mon, 20 Apr 2026 18:29:32 +0200 Subject: [PATCH 03/12] feat: share Delta with callback --- crdt.go | 28 +++++- crdt_test.go | 28 ++++-- set.go | 71 ++++++++++----- set_test.go | 241 +++++++++++++++++++++++++++++++++++++++++++++------ 4 files changed, 309 insertions(+), 59 deletions(-) diff --git a/crdt.go b/crdt.go index 48e82ed..b2a61b2 100644 --- a/crdt.go +++ b/crdt.go @@ -76,6 +76,30 @@ type SessionDAGService interface { Session(context.Context) ipld.NodeGetter } +// PutEvent describes a materialised-view update delivered to OnPut. +// Delta is the triggering delta: the one whose merge caused the state change. +// For partial-tombstone puts (where a tombstone removed the previous winner +// and a surviving element took over), Delta is the tombstone delta, not the +// older delta that originally wrote the surviving value. Delta is nil when +// the update did not originate from a merged delta (e.g. PurgeDAG). +// Consumers must not retain Delta past the callback or mutate it. +type PutEvent struct { + Key ds.Key + OldValue []byte + NewValue []byte + Delta Delta +} + +// DeleteEvent describes a materialised-view removal delivered to OnDelete. +// Delta is the tombstone delta that triggered the removal, or nil when the +// removal did not originate from a merged delta (e.g. PurgeDAG). Consumers +// must not retain Delta past the callback or mutate it. +type DeleteEvent struct { + Key ds.Key + LastValue []byte + Delta Delta +} + // Options holds configurable values for Datastore. type Options struct { Logger logging.StandardLogger @@ -95,7 +119,7 @@ type Options struct { // read per put. Mutually exclusive with PutHook. Default: nil. // The callback is invoked while internal locks are held; it must not // call back into the Datastore or it will deadlock. - OnPut func(k ds.Key, newVal, oldVal []byte) + OnPut func(PutEvent) // DeleteHook is triggered whenever a version of an element is // successfully removed from the datastore (either by a local or remote // update). Unordered and concurrent updates may result in the DeleteHook @@ -109,7 +133,7 @@ type Options struct { // delete. Mutually exclusive with DeleteHook. Default: nil. // The callback is invoked while internal locks are held; it must not // call back into the Datastore or it will deadlock. - OnDelete func(k ds.Key, lastVal []byte) + OnDelete func(DeleteEvent) // NumWorkers specifies the number of workers ready to // retrieve and merge deltas while walking the DAGs. Default: // 5. diff --git a/crdt_test.go b/crdt_test.go index 7042f82..b395acf 100644 --- a/crdt_test.go +++ b/crdt_test.go @@ -607,14 +607,19 @@ func TestCRDTOnPut(t *testing.T) { key ds.Key newVal []byte oldVal []byte + prio uint64 } var calls []hookCall var mu sync.Mutex opts := DefaultOptions() - opts.OnPut = func(k ds.Key, newVal, oldVal []byte) { + opts.OnPut = func(e PutEvent) { mu.Lock() - calls = append(calls, hookCall{k, newVal, oldVal}) + var prio uint64 + if e.Delta != nil { + prio = e.Delta.GetPriority() + } + calls = append(calls, hookCall{e.Key, e.NewValue, e.OldValue, prio}) mu.Unlock() } @@ -641,6 +646,9 @@ func TestCRDTOnPut(t *testing.T) { if c.oldVal != nil { t.Errorf("call[%d]: expected oldVal nil on first put, got %q", i, c.oldVal) } + if c.prio == 0 { + t.Errorf("call[%d]: expected non-zero delta priority, got 0 (delta not forwarded?)", i) + } } calls = nil mu.Unlock() @@ -671,16 +679,17 @@ func TestCRDTOnDelete(t *testing.T) { t.Parallel() synctest.Test(t, func(t *testing.T) { type hookCall struct { - key ds.Key - lastVal []byte + key ds.Key + lastVal []byte + hasDelta bool } var calls []hookCall var mu sync.Mutex opts := DefaultOptions() - opts.OnDelete = func(k ds.Key, lastVal []byte) { + opts.OnDelete = func(e DeleteEvent) { mu.Lock() - calls = append(calls, hookCall{k, lastVal}) + calls = append(calls, hookCall{e.Key, e.LastValue, e.Delta != nil}) mu.Unlock() } @@ -707,6 +716,9 @@ func TestCRDTOnDelete(t *testing.T) { if string(c.lastVal) != "hello" { t.Errorf("call[%d]: expected lastVal %q, got %q", i, "hello", c.lastVal) } + if !c.hasDelta { + t.Errorf("call[%d]: expected non-nil Delta from tombstone merge, got nil", i) + } } calls = nil mu.Unlock() @@ -738,7 +750,7 @@ func TestCRDTHooksMutuallyExclusive(t *testing.T) { t.Cleanup(cancelBcasts) opts := DefaultOptions() opts.PutHook = func(ds.Key, []byte) {} - opts.OnPut = func(ds.Key, []byte, []byte) {} + opts.OnPut = func(PutEvent) {} _, err := New(store, namespace, dagService, bcasts[0], opts) if err == nil { t.Fatal("expected error when both PutHook and OnPut are set") @@ -750,7 +762,7 @@ func TestCRDTHooksMutuallyExclusive(t *testing.T) { t.Cleanup(cancelBcasts) opts := DefaultOptions() opts.DeleteHook = func(ds.Key) {} - opts.OnDelete = func(ds.Key, []byte) {} + opts.OnDelete = func(DeleteEvent) {} _, err := New(store, namespace, dagService, bcasts[0], opts) if err == nil { t.Fatal("expected error when both DeleteHook and OnDelete are set") diff --git a/set.go b/set.go index 53b2a44..f327093 100644 --- a/set.go +++ b/set.go @@ -40,9 +40,9 @@ type Set interface { // putHook or onPut may be set; likewise for deleteHook and onDelete. type setHooks struct { putHook func(key ds.Key, v []byte) - onPut func(key ds.Key, newVal, oldVal []byte) + onPut func(PutEvent) deleteHook func(key ds.Key) - onDelete func(key ds.Key, lastVal []byte) + onDelete func(DeleteEvent) } // set implements an Add-Wins Observed-Remove Set using delta-CRDTs @@ -85,11 +85,18 @@ func newCRDTSet( return set, nil } -// triggerPutHook calls the appropriate put hook with newVal and oldVal. -// oldVal must have been fetched from the store before the write. -func (s *set) triggerPutHook(key string, newVal, oldVal []byte) { +// triggerPutHook calls the appropriate put hook with newVal, oldVal, and the +// triggering delta. oldVal must have been fetched from the store before the +// write. delta is the merged delta that caused the update, or nil when the +// update did not originate from a merged delta (e.g. PurgeDAG). +func (s *set) triggerPutHook(key string, newVal, oldVal []byte, delta Delta) { if s.hooks.onPut != nil { - s.hooks.onPut(ds.NewKey(key), newVal, oldVal) + s.hooks.onPut(PutEvent{ + Key: ds.NewKey(key), + OldValue: oldVal, + NewValue: newVal, + Delta: delta, + }) return } if s.hooks.putHook != nil { @@ -97,11 +104,16 @@ func (s *set) triggerPutHook(key string, newVal, oldVal []byte) { } } -// triggerDeleteHook calls the appropriate delete hook with lastVal, -// the value that was stored before the deletion. -func (s *set) triggerDeleteHook(key string, lastVal []byte) { +// triggerDeleteHook calls the appropriate delete hook with lastVal (the value +// stored before the deletion) and the triggering tombstone delta, or nil when +// the removal did not originate from a merged delta (e.g. PurgeDAG). +func (s *set) triggerDeleteHook(key string, lastVal []byte, delta Delta) { if s.hooks.onDelete != nil { - s.hooks.onDelete(ds.NewKey(key), lastVal) + s.hooks.onDelete(DeleteEvent{ + Key: ds.NewKey(key), + LastValue: lastVal, + Delta: delta, + }) return } if s.hooks.deleteHook != nil { @@ -370,13 +382,20 @@ func (s *set) setPriority(ctx context.Context, writeStore ds.Write, key string, // sets a value if priority is higher. When equal, it sets if the // value is lexicographically higher than the current value. -func (s *set) setValue(ctx context.Context, writeStore ds.Write, key, id string, value []byte, prio uint64) error { +// delta is the triggering delta forwarded to the put hook when the write is +// considered the prevalent value. delta must not be nil; its priority drives +// the write decision. +func (s *set) setValue(ctx context.Context, writeStore ds.Write, key, id string, value []byte, delta Delta) error { + if delta == nil { + return errors.New("setValue: delta must not be nil") + } // If this key was tombstoned already, do not store/update the value. deleted, err := s.inTombsKeyID(ctx, key, id) if err != nil || deleted { return err } + prio := delta.GetPriority() curPrio, err := s.getPriority(ctx, key) if err != nil { return err @@ -418,7 +437,7 @@ func (s *set) setValue(ctx context.Context, writeStore ds.Write, key, id string, } // trigger add hook - s.triggerPutHook(key, value, oldVal) + s.triggerPutHook(key, value, oldVal, delta) return nil } @@ -543,13 +562,19 @@ NEXT: // but with the batching optimization the locks would need to be hold until // the batch is written), and one lock per key might be way worse than a single // global lock in the end. -func (s *set) putElems(ctx context.Context, elems []*pb.Element, id string, prio uint64) error { +// +// delta is the triggering delta; it supplies the write priority and is +// forwarded to the put hook. delta must not be nil when elems is non-empty. +func (s *set) putElems(ctx context.Context, elems []*pb.Element, id string, delta Delta) error { s.putElemsMux.Lock() defer s.putElemsMux.Unlock() if len(elems) == 0 { return nil } + if delta == nil { + return errors.New("putElems: delta must not be nil when elems is non-empty") + } var store ds.Write = s.store var err error @@ -574,7 +599,7 @@ func (s *set) putElems(ctx context.Context, elems []*pb.Element, id string, prio // update the value if applicable: // * higher priority than we currently have. // * not tombstoned before. - err = s.setValue(ctx, store, key, id, e.GetValue(), prio) + err = s.setValue(ctx, store, key, id, e.GetValue(), delta) if err != nil { return err } @@ -589,7 +614,11 @@ func (s *set) putElems(ctx context.Context, elems []*pb.Element, id string, prio return nil } -func (s *set) putTombs(ctx context.Context, tombs []*pb.Element) error { +// putTombs applies tombstones and recomputes winners for the affected keys. +// delta is the tombstone delta that triggered the removal and is forwarded to +// put/delete hooks. delta may be nil when the caller has no originating delta +// (e.g. the purge path), since it is only used for hook forwarding. +func (s *set) putTombs(ctx context.Context, tombs []*pb.Element, delta Delta) error { if len(tombs) == 0 { return nil } @@ -700,13 +729,13 @@ func (s *set) putTombs(ctx context.Context, tombs []*pb.Element) error { if bytes.Equal(newVal, oldVal) { continue // value unchanged, skip hook } - s.triggerPutHook(del, newVal, oldVal) + s.triggerPutHook(del, newVal, oldVal, delta) } else { lastVal, ok := prevVals[del] if !ok { continue // key had no value before tombstone, nothing to notify } - s.triggerDeleteHook(del, lastVal) + s.triggerDeleteHook(del, lastVal, delta) } } } @@ -725,12 +754,12 @@ func (s *set) Merge(ctx context.Context, d Delta, id string) error { return err } - err = s.putTombs(ctx, tombs) + err = s.putTombs(ctx, tombs, d) if err != nil { return err } - return s.putElems(ctx, elems, id, d.GetPriority()) + return s.putElems(ctx, elems, id, d) } // currently unused @@ -876,7 +905,7 @@ func (s *set) purgeKeyBlocks(ctx context.Context, key string, blockCIDs map[cid. if err := errors.Join(errs...); err != nil { return err } - s.triggerDeleteHook(key, oldVal) + s.triggerDeleteHook(key, oldVal, nil) } else { if err := s.store.Put(ctx, valueK, bestVal); err != nil { return err @@ -884,7 +913,7 @@ func (s *set) purgeKeyBlocks(ctx context.Context, key string, blockCIDs map[cid. if err := s.setPriority(ctx, s.store, key, bestPrio); err != nil { return err } - s.triggerPutHook(key, bestVal, oldVal) + s.triggerPutHook(key, bestVal, oldVal, nil) } return nil diff --git a/set_test.go b/set_test.go index 33ddde0..3b2a771 100644 --- a/set_test.go +++ b/set_test.go @@ -5,6 +5,7 @@ import ( dshelp "github.com/ipfs/boxo/datastore/dshelp" mdutils "github.com/ipfs/boxo/ipld/merkledag/test" + cid "github.com/ipfs/go-cid" ds "github.com/ipfs/go-datastore" dssync "github.com/ipfs/go-datastore/sync" pb "github.com/ipfs/go-ds-crdt/pb" @@ -44,7 +45,7 @@ func addElem(t *testing.T, s *set, dagService ipld.DAGService, key string, value if err != nil { t.Fatalf("GetElements: %v", err) } - if err := s.putElems(ctx, elems, blockKey, prio); err != nil { + if err := s.putElems(ctx, elems, blockKey, d); err != nil { t.Fatalf("putElems: %v", err) } return blockKey @@ -60,10 +61,10 @@ func TestPutTombsEmpty(t *testing.T) { deleteHook: func(ds.Key) { fired = true }, }) - if err := s.putTombs(ctx, nil); err != nil { + if err := s.putTombs(ctx, nil, nil); err != nil { t.Fatal(err) } - if err := s.putTombs(ctx, []*pb.Element{}); err != nil { + if err := s.putTombs(ctx, []*pb.Element{}, nil); err != nil { t.Fatal(err) } if fired { @@ -83,7 +84,7 @@ func TestPutTombsFullDelete(t *testing.T) { s := newTestSet(t, dag, setHooks{}) id := addElem(t, s, dag, "foo", []byte("hello"), 1) - if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id}}); err != nil { + if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id}}, nil); err != nil { t.Fatal(err) } if _, err := s.store.Get(ctx, s.valueKey("foo")); err != ds.ErrNotFound { @@ -105,7 +106,7 @@ func TestPutTombsFullDelete(t *testing.T) { s := newTestSet(t, dag, setHooks{deleteHook: func(k ds.Key) { keys = append(keys, k) }}) id := addElem(t, s, dag, "foo", []byte("hello"), 1) - if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id}}); err != nil { + if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id}}, nil); err != nil { t.Fatal(err) } if len(keys) != 1 || keys[0].String() != ds.NewKey("foo").String() { @@ -120,11 +121,11 @@ func TestPutTombsFullDelete(t *testing.T) { var gotKey ds.Key var gotVal []byte s := newTestSet(t, dag, setHooks{ - onDelete: func(k ds.Key, v []byte) { gotKey, gotVal = k, v }, + onDelete: func(e DeleteEvent) { gotKey, gotVal = e.Key, e.LastValue }, }) id := addElem(t, s, dag, "foo", []byte("hello"), 1) - if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id}}); err != nil { + if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id}}, nil); err != nil { t.Fatal(err) } if gotKey.String() != ds.NewKey("foo").String() { @@ -152,7 +153,7 @@ func TestPutTombsNilValue(t *testing.T) { }) id := addElem(t, s, dag, "foo", nil, 1) - if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id}}); err != nil { + if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id}}, nil); err != nil { t.Fatal(err) } if !deleteFired { @@ -168,11 +169,11 @@ func TestPutTombsNilValue(t *testing.T) { var gotVal []byte var called bool s := newTestSet(t, dag, setHooks{ - onDelete: func(k ds.Key, v []byte) { gotKey, gotVal, called = k, v, true }, + onDelete: func(e DeleteEvent) { gotKey, gotVal, called = e.Key, e.LastValue, true }, }) id := addElem(t, s, dag, "foo", nil, 1) - if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id}}); err != nil { + if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id}}, nil); err != nil { t.Fatal(err) } if !called { @@ -194,8 +195,8 @@ func TestPutTombsNilValue(t *testing.T) { var deleteLastVal []byte var putFired bool s := newTestSet(t, dag, setHooks{ - onPut: func(ds.Key, []byte, []byte) { putFired = true }, - onDelete: func(_ ds.Key, v []byte) { deleteFired = true; deleteLastVal = v }, + onPut: func(PutEvent) { putFired = true }, + onDelete: func(e DeleteEvent) { deleteFired = true; deleteLastVal = e.LastValue }, }) // "aaa" wins over nil lexicographically; store value is "aaa". id1 := addElem(t, s, dag, "foo", nil, 1) // loses @@ -211,7 +212,7 @@ func TestPutTombsNilValue(t *testing.T) { // the store, and fires onDelete. As a result InSet returns false even // though a surviving element exists. Fixing this requires findBestValue // to return a separate found bool alongside the value. - if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id2}}); err != nil { + if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id2}}, nil); err != nil { t.Fatal(err) } _ = id1 @@ -242,7 +243,7 @@ func TestPutTombsPartialTombstone(t *testing.T) { id1 := addElem(t, s, dag, "foo", []byte("aaa"), 1) addElem(t, s, dag, "foo", []byte("bbb"), 1) - if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id1}}); err != nil { + if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id1}}, nil); err != nil { t.Fatal(err) } val, err := s.Element(ctx, "foo") @@ -268,7 +269,7 @@ func TestPutTombsPartialTombstone(t *testing.T) { putFired = false // Tombstone the loser: "bbb" remains the winner, no observable change. - if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id1}}); err != nil { + if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id1}}, nil); err != nil { t.Fatal(err) } if putFired || deleteFired { @@ -292,7 +293,7 @@ func TestPutTombsPartialTombstone(t *testing.T) { // Tombstone the current winner ("bbb"). "aaa" becomes the new winner, // so the value changes and putHook must fire. - if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id2}}); err != nil { + if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id2}}, nil); err != nil { t.Fatal(err) } if deleteFired { @@ -310,7 +311,7 @@ func TestPutTombsPartialTombstone(t *testing.T) { type call struct{ newVal, oldVal []byte } var calls []call s := newTestSet(t, dag, setHooks{ - onPut: func(_ ds.Key, n, o []byte) { calls = append(calls, call{n, o}) }, + onPut: func(e PutEvent) { calls = append(calls, call{e.NewValue, e.OldValue}) }, }) addElem(t, s, dag, "foo", []byte("aaa"), 1) id2 := addElem(t, s, dag, "foo", []byte("bbb"), 1) // "bbb" wins, is current value @@ -318,7 +319,7 @@ func TestPutTombsPartialTombstone(t *testing.T) { // Tombstone the current winner ("bbb"). "aaa" becomes the new winner: // onPut must fire with newVal="aaa", oldVal="bbb". - if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id2}}); err != nil { + if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id2}}, nil); err != nil { t.Fatal(err) } if len(calls) != 1 { @@ -342,15 +343,15 @@ func TestPutTombsMultipleKeys(t *testing.T) { deleted := make(map[string][]byte) var onPutCalled bool s := newTestSet(t, dag, setHooks{ - onDelete: func(k ds.Key, v []byte) { deleted[k.String()] = v }, - onPut: func(ds.Key, []byte, []byte) { onPutCalled = true }, + onDelete: func(e DeleteEvent) { deleted[e.Key.String()] = e.LastValue }, + onPut: func(PutEvent) { onPutCalled = true }, }) id1 := addElem(t, s, dag, "key1", []byte("val1"), 1) id2 := addElem(t, s, dag, "key2", []byte("val2"), 1) onPutCalled = false // discard calls from addElem setup tombs := []*pb.Element{{Key: "key1", Id: id1}, {Key: "key2", Id: id2}} - if err := s.putTombs(ctx, tombs); err != nil { + if err := s.putTombs(ctx, tombs, nil); err != nil { t.Fatal(err) } if len(deleted) != 2 { @@ -375,11 +376,11 @@ func TestPutTombsNonExistentKey(t *testing.T) { ctx := t.Context() var putFired, deleteFired bool s := newTestSet(t, mdutils.Mock(), setHooks{ - onPut: func(ds.Key, []byte, []byte) { putFired = true }, - onDelete: func(ds.Key, []byte) { deleteFired = true }, + onPut: func(PutEvent) { putFired = true }, + onDelete: func(DeleteEvent) { deleteFired = true }, }) - if err := s.putTombs(ctx, []*pb.Element{{Key: "ghost", Id: "fakeid"}}); err != nil { + if err := s.putTombs(ctx, []*pb.Element{{Key: "ghost", Id: "fakeid"}}, nil); err != nil { t.Fatalf("putTombs: %v", err) } inTomb, err := s.inTombsKeyID(ctx, "ghost", "fakeid") @@ -411,14 +412,14 @@ func TestPutTombsPrevValsFirstEncounterOnly(t *testing.T) { var lastVals [][]byte s := newTestSet(t, dag, setHooks{ putHook: func(ds.Key, []byte) { putFired = true }, - onDelete: func(_ ds.Key, v []byte) { lastVals = append(lastVals, v) }, + onDelete: func(e DeleteEvent) { lastVals = append(lastVals, e.LastValue) }, }) id1 := addElem(t, s, dag, "foo", []byte("first"), 1) id2 := addElem(t, s, dag, "foo", []byte("second"), 2) // wins (higher prio) putFired = false tombs := []*pb.Element{{Key: "foo", Id: id1}, {Key: "foo", Id: id2}} - if err := s.putTombs(ctx, tombs); err != nil { + if err := s.putTombs(ctx, tombs, nil); err != nil { t.Fatal(err) } if putFired { @@ -444,7 +445,7 @@ func TestPutTombsIdempotent(t *testing.T) { tombs := []*pb.Element{{Key: "foo", Id: id}} for range 5 { - if err := s.putTombs(ctx, tombs); err != nil { + if err := s.putTombs(ctx, tombs, nil); err != nil { t.Fatal(err) } } @@ -456,6 +457,123 @@ func TestPutTombsIdempotent(t *testing.T) { } } +// TestPutElemsDeltaForwarded verifies that the Delta passed to putElems is +// forwarded identically to the onPut hook. +func TestPutElemsDeltaForwarded(t *testing.T) { + t.Parallel() + ctx := t.Context() + var gotDelta Delta + s := newTestSet(t, mdutils.Mock(), setHooks{ + onPut: func(e PutEvent) { gotDelta = e.Delta }, + }) + + var prio uint64 = 42 + d := &pbDelta{Delta: &pb.Delta{DagName: "dag-test", Priority: prio}} + if err := s.putElems(ctx, []*pb.Element{{Key: "foo", Value: []byte("v")}}, "block1", d); err != nil { + t.Fatal(err) + } + if gotDelta != d { + t.Fatalf("onPut delta pointer mismatch: got %p want %p", gotDelta, d) + } + if gotDelta.GetPriority() != prio { + t.Errorf("forwarded delta priority = %d, want %d", gotDelta.GetPriority(), prio) + } + if gotDelta.GetDagName() != "dag-test" { + t.Errorf("forwarded delta dagName = %q, want dag-test", gotDelta.GetDagName()) + } +} + +// TestPutTombsFullDeleteDeltaForwarded verifies that a tombstone-only delta is +// forwarded to the onDelete hook when a key is fully deleted. +func TestPutTombsFullDeleteDeltaForwarded(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + var gotDelta Delta + s := newTestSet(t, dag, setHooks{ + onDelete: func(e DeleteEvent) { gotDelta = e.Delta }, + }) + id := addElem(t, s, dag, "foo", []byte("hello"), 1) + + tombDelta := &pbDelta{Delta: &pb.Delta{DagName: "tomb-dag"}} + tombs := []*pb.Element{{Key: "foo", Id: id}} + if err := s.putTombs(ctx, tombs, tombDelta); err != nil { + t.Fatal(err) + } + if gotDelta != tombDelta { + t.Fatalf("onDelete delta pointer mismatch: got %p want %p", gotDelta, tombDelta) + } + if gotDelta.GetDagName() != "tomb-dag" { + t.Errorf("forwarded delta dagName = %q, want tomb-dag", gotDelta.GetDagName()) + } +} + +// TestPutTombsPartialPutDeltaForwarded verifies that when a tombstone removes +// the current winner and a surviving element takes over, the onPut hook +// receives the tombstone delta (the one that triggered the MV change), not +// the original put delta. +func TestPutTombsPartialPutDeltaForwarded(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + var gotEvent PutEvent + // Only capture the hook call triggered by putTombs; ignore the addElem puts. + var capture bool + s := newTestSet(t, dag, setHooks{ + onPut: func(e PutEvent) { + if capture { + gotEvent = e + } + }, + }) + // Seed two elements; "aaa" at prio 2 is the current winner. + addElem(t, s, dag, "foo", []byte("zzz"), 1) + id2 := addElem(t, s, dag, "foo", []byte("aaa"), 2) + + capture = true + tombDelta := &pbDelta{Delta: &pb.Delta{DagName: "tomb-dag", Priority: 3}} + // Tombstone the winner so "zzz" takes over — that changes the MV and fires onPut. + if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id2}}, tombDelta); err != nil { + t.Fatal(err) + } + if gotEvent.Delta != tombDelta { + t.Fatalf("partial-put onPut delta should be the tombstone delta: got %p want %p", gotEvent.Delta, tombDelta) + } + if string(gotEvent.NewValue) != "zzz" || string(gotEvent.OldValue) != "aaa" { + t.Errorf("onPut values = (new=%q, old=%q), want (new=zzz, old=aaa)", gotEvent.NewValue, gotEvent.OldValue) + } +} + +// TestCustomDeltaTypeAssert verifies that callbacks can type-assert the Delta +// back to a concrete implementation to reach application-specific fields +func TestCustomDeltaTypeAssert(t *testing.T) { + t.Parallel() + ctx := t.Context() + var gotCutsomField int64 + s := newTestSet(t, mdutils.Mock(), setHooks{ + onPut: func(e PutEvent) { + if cd, ok := e.Delta.(*customDelta); ok { + gotCutsomField = cd.CustomField + } + }, + }) + + d := &customDelta{pbDelta: pbDelta{Delta: &pb.Delta{Priority: 1}}, CustomField: 1713456789} + if err := s.putElems(ctx, []*pb.Element{{Key: "foo", Value: []byte("v")}}, "block1", d); err != nil { + t.Fatal(err) + } + if gotCutsomField != 1713456789 { + t.Errorf("type-asserted custom field = %d, want 1713456789", gotCutsomField) + } +} + +// customDelta embeds pbDelta and adds an application-specific field, mimicking +// how an external application supplies its own DeltaFactory. +type customDelta struct { + pbDelta + CustomField int64 +} + // TestPutTombsHigherPriorityWins verifies that the higher-priority element // survives when the lower-priority one is tombstoned. func TestPutTombsHigherPriorityWins(t *testing.T) { @@ -466,7 +584,7 @@ func TestPutTombsHigherPriorityWins(t *testing.T) { id1 := addElem(t, s, dag, "foo", []byte("zzz"), 1) // high lex, low prio addElem(t, s, dag, "foo", []byte("aaa"), 2) // low lex, high prio - if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id1}}); err != nil { + if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id1}}, nil); err != nil { t.Fatal(err) } val, err := s.Element(ctx, "foo") @@ -477,3 +595,70 @@ func TestPutTombsHigherPriorityWins(t *testing.T) { t.Errorf("surviving value = %q, want aaa (high-priority element)", val) } } + +// TestPurgeKeyBlocksDeltaNil verifies that hooks fired from the purge path +// receive a nil Delta, since no originating delta triggered the state change. +func TestPurgeKeyBlocksDeltaNil(t *testing.T) { + t.Parallel() + + t.Run("full purge fires onDelete with nil delta", func(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + var gotEvent DeleteEvent + var fired bool + s := newTestSet(t, dag, setHooks{ + onDelete: func(e DeleteEvent) { gotEvent = e; fired = true }, + }) + id := addElem(t, s, dag, "foo", []byte("hello"), 1) + + c := blockIDToCid(t, id) + if err := s.purgeKeyBlocks(ctx, "foo", map[cid.Cid]struct{}{c: {}}, true, false); err != nil { + t.Fatal(err) + } + if !fired { + t.Fatal("onDelete should fire for full purge") + } + if gotEvent.Delta != nil { + t.Errorf("onDelete delta = %v, want nil (no originating delta on purge path)", gotEvent.Delta) + } + if string(gotEvent.LastValue) != "hello" { + t.Errorf("onDelete lastValue = %q, want hello", gotEvent.LastValue) + } + }) + + t.Run("partial purge fires onPut with nil delta", func(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + var gotEvent PutEvent + var fired bool + s := newTestSet(t, dag, setHooks{ + onPut: func(e PutEvent) { gotEvent = e; fired = true }, + }) + id1 := addElem(t, s, dag, "foo", []byte("zzz"), 1) + addElem(t, s, dag, "foo", []byte("aaa"), 2) + + c := blockIDToCid(t, id1) + if err := s.purgeKeyBlocks(ctx, "foo", map[cid.Cid]struct{}{c: {}}, true, false); err != nil { + t.Fatal(err) + } + if !fired { + t.Fatal("onPut should fire when partial purge changes the winner") + } + if gotEvent.Delta != nil { + t.Errorf("onPut delta = %v, want nil (no originating delta on purge path)", gotEvent.Delta) + } + }) +} + +// blockIDToCid converts the blockKey returned by addElem (a datastore-keyed +// multihash) back into a CID, for use with purgeKeyBlocks. +func blockIDToCid(t *testing.T, blockID string) cid.Cid { + t.Helper() + mhash, err := dshelp.DsKeyToMultihash(ds.NewKey(blockID)) + if err != nil { + t.Fatalf("DsKeyToMultihash: %v", err) + } + return cid.NewCidV1(cid.DagProtobuf, mhash) +} From 94ade4a63c9742c9eb43f4aae130b7c4cdf008ea Mon Sep 17 00:00:00 2001 From: guillaumemichel Date: Tue, 21 Apr 2026 17:05:01 +0200 Subject: [PATCH 04/12] refactor: consolidate put/delete hooks with event structs --- crdt.go | 111 +++++++++++++----------- crdt_test.go | 62 ++++--------- examples/globaldb/globaldb.go | 8 +- set.go | 130 ++++++++++++++-------------- set_test.go | 158 ++++++++++++++++++---------------- 5 files changed, 234 insertions(+), 235 deletions(-) diff --git a/crdt.go b/crdt.go index b2a61b2..9df46a7 100644 --- a/crdt.go +++ b/crdt.go @@ -76,28 +76,35 @@ type SessionDAGService interface { Session(context.Context) ipld.NodeGetter } -// PutEvent describes a materialised-view update delivered to OnPut. -// Delta is the triggering delta: the one whose merge caused the state change. -// For partial-tombstone puts (where a tombstone removed the previous winner -// and a surviving element took over), Delta is the tombstone delta, not the -// older delta that originally wrote the surviving value. Delta is nil when -// the update did not originate from a merged delta (e.g. PurgeDAG). -// Consumers must not retain Delta past the callback or mutate it. +// PutEvent describes a materialised-view update delivered to PutHook. type PutEvent struct { - Key ds.Key + // Key whose value was updated. + Key ds.Key + // OldValue is the previous value for the key, if any. It is populated only + // when HookLoadPreviousValue is true, otherwise it is nil. OldValue []byte + // NewValue is the new value for the key after the update. NewValue []byte - Delta Delta + // Delta is the triggering delta: the one whose merge caused the state + // change. For partial-tombstone puts (where a tombstone removed the previous + // winner and a surviving element took over), Delta is the tombstone delta, + // not the older delta that originally wrote the surviving value. Delta is + // nil when the update did not originate from a merged delta (e.g. PurgeDAG). + // Consumers must not retain Delta past the callback or mutate it. + Delta Delta } -// DeleteEvent describes a materialised-view removal delivered to OnDelete. -// Delta is the tombstone delta that triggered the removal, or nil when the -// removal did not originate from a merged delta (e.g. PurgeDAG). Consumers -// must not retain Delta past the callback or mutate it. +// DeleteEvent describes a materialised-view removal delivered to DeleteHook. type DeleteEvent struct { - Key ds.Key + // Key whose value was updated. + Key ds.Key + // LastValue is the previous value for the key, if any. It is populated only + // when HookLoadPreviousValue is true, otherwise it is nil. LastValue []byte - Delta Delta + // Delta is the tombstone delta that triggered the removal, or nil when the + // removal did not originate from a merged delta (e.g. PurgeDAG). Consumers + // must not retain Delta past the callback or mutate it. + Delta Delta } // Options holds configurable values for Datastore. @@ -111,29 +118,37 @@ type Options struct { RebroadcastInterval time.Duration // PutHook is triggered whenever an element is successfully added to the // datastore (either by a local or remote update), and only when that - // addition is considered the prevalent value. Default: nil. - // Mutually exclusive with OnPut. - PutHook func(k ds.Key, v []byte) - // OnPut is like PutHook but also receives the previous value for the key - // (nil if the key did not exist before). This incurs an extra datastore - // read per put. Mutually exclusive with PutHook. Default: nil. + // addition is considered the prevalent value. For partial-tombstone puts + // (where a tombstone removed the previous winner and a surviving element + // took over), the hook fires with the surviving value. Default: nil. + // + // PutEvent.OldValue is populated only when HookLoadPreviousValue is true, + // otherwise it is nil. When HookLoadPreviousValue is true, the hook is not + // fired for partial tombstones where the winning value did not change. + // // The callback is invoked while internal locks are held; it must not // call back into the Datastore or it will deadlock. - OnPut func(PutEvent) - // DeleteHook is triggered whenever a version of an element is - // successfully removed from the datastore (either by a local or remote - // update). Unordered and concurrent updates may result in the DeleteHook - // being triggered even though the element is still present in the - // datastore because it was re-added or not fully tombstoned. If that is - // relevant, use Has() to check if the removed element is still part of - // the datastore. Default: nil. Mutually exclusive with OnDelete. - DeleteHook func(k ds.Key) - // OnDelete is like DeleteHook but also receives the last known value for - // the key before it was removed. This incurs an extra datastore read per - // delete. Mutually exclusive with DeleteHook. Default: nil. + PutHook func(PutEvent) + // DeleteHook is triggered whenever an element is fully removed from the + // datastore (either by a local or remote update), i.e. when no surviving + // element exists for the key after tombstone processing. Default: nil. + // + // DeleteEvent.LastValue is populated only when HookLoadPreviousValue is true, + // otherwise it is nil. When HookLoadPreviousValue is true, the hook is not + // fired for tombstones targeting keys that had no prior value in the + // datastore. + // // The callback is invoked while internal locks are held; it must not // call back into the Datastore or it will deadlock. - OnDelete func(DeleteEvent) + DeleteHook func(DeleteEvent) + // HookLoadPreviousValue controls whether PutHook/DeleteHook receive the + // previous value for the key. When true, the datastore is read before + // each hook-relevant write so that PutEvent.OldValue and + // DeleteEvent.LastValue can be populated; this incurs one extra read per + // triggered hook. When true, the hook is also skipped when the observed + // value would not actually change (see PutHook/DeleteHook doc). Default: + // false. + HookLoadPreviousValue bool // NumWorkers specifies the number of workers ready to // retrieve and merge deltas while walking the DAGs. Default: // 5. @@ -219,14 +234,13 @@ func (opts *Options) verify() error { // DefaultOptions initializes an Options object with sensible defaults. func DefaultOptions() *Options { return &Options{ - Logger: logging.Logger("crdt"), - RebroadcastInterval: time.Minute, - PutHook: nil, - OnPut: nil, - DeleteHook: nil, - OnDelete: nil, - NumWorkers: 5, - DAGSyncerTimeout: 5 * time.Minute, + Logger: logging.Logger("crdt"), + RebroadcastInterval: time.Minute, + PutHook: nil, + DeleteHook: nil, + HookLoadPreviousValue: false, + NumWorkers: 5, + DAGSyncerTimeout: 5 * time.Minute, // always keeping // https://github.com/libp2p/go-libp2p-core/blob/master/network/network.go#L23 // in sight @@ -330,17 +344,10 @@ func New( return nil, err } - if opts.PutHook != nil && opts.OnPut != nil { - return nil, errors.New("PutHook and OnPut are mutually exclusive") - } - if opts.DeleteHook != nil && opts.OnDelete != nil { - return nil, errors.New("DeleteHook and OnDelete are mutually exclusive") - } hooks := setHooks{ - putHook: opts.PutHook, - onPut: opts.OnPut, - deleteHook: opts.DeleteHook, - onDelete: opts.OnDelete, + putHook: opts.PutHook, + deleteHook: opts.DeleteHook, + hookLoadPreviousValue: opts.HookLoadPreviousValue, } // /set diff --git a/crdt_test.go b/crdt_test.go index b395acf..b2b463d 100644 --- a/crdt_test.go +++ b/crdt_test.go @@ -573,10 +573,10 @@ func TestCRDTHooks(t *testing.T) { var deleted int64 opts := DefaultOptions() - opts.PutHook = func(k ds.Key, v []byte) { + opts.PutHook = func(PutEvent) { atomic.AddInt64(&put, 1) } - opts.DeleteHook = func(k ds.Key) { + opts.DeleteHook = func(DeleteEvent) { atomic.AddInt64(&deleted, 1) } @@ -600,7 +600,9 @@ func TestCRDTHooks(t *testing.T) { }) } -func TestCRDTOnPut(t *testing.T) { +// TestCRDTPutHookLoadsPreviousValue verifies that PutHook receives OldValue +// populated when HookLoadPreviousValue is enabled. +func TestCRDTPutHookLoadsPreviousValue(t *testing.T) { t.Parallel() synctest.Test(t, func(t *testing.T) { type hookCall struct { @@ -613,7 +615,8 @@ func TestCRDTOnPut(t *testing.T) { var mu sync.Mutex opts := DefaultOptions() - opts.OnPut = func(e PutEvent) { + opts.HookLoadPreviousValue = true + opts.PutHook = func(e PutEvent) { mu.Lock() var prio uint64 if e.Delta != nil { @@ -629,7 +632,7 @@ func TestCRDTOnPut(t *testing.T) { k := ds.NewKey("/testkey") ctx := t.Context() - // First put: all replicas should fire onPut with newVal="first", oldVal=nil. + // First put: all replicas should fire PutHook with newVal="first", oldVal=nil. if err := replicas[0].Put(ctx, k, []byte("first")); err != nil { t.Fatal(err) } @@ -637,7 +640,7 @@ func TestCRDTOnPut(t *testing.T) { mu.Lock() if len(calls) != len(replicas) { - t.Errorf("expected %d OnPut calls (one per replica), got %d", len(replicas), len(calls)) + t.Errorf("expected %d PutHook calls (one per replica), got %d", len(replicas), len(calls)) } for i, c := range calls { if string(c.newVal) != "first" { @@ -653,7 +656,7 @@ func TestCRDTOnPut(t *testing.T) { calls = nil mu.Unlock() - // Second put: all replicas should fire onPut with newVal="second", oldVal="first". + // Second put: all replicas should fire PutHook with newVal="second", oldVal="first". if err := replicas[0].Put(ctx, k, []byte("second")); err != nil { t.Fatal(err) } @@ -662,7 +665,7 @@ func TestCRDTOnPut(t *testing.T) { mu.Lock() defer mu.Unlock() if len(calls) != len(replicas) { - t.Errorf("expected %d OnPut calls on second put, got %d", len(replicas), len(calls)) + t.Errorf("expected %d PutHook calls on second put, got %d", len(replicas), len(calls)) } for i, c := range calls { if string(c.newVal) != "second" { @@ -675,7 +678,10 @@ func TestCRDTOnPut(t *testing.T) { }) } -func TestCRDTOnDelete(t *testing.T) { +// TestCRDTDeleteHookLoadsPreviousValue verifies that DeleteHook receives +// LastValue populated when HookLoadPreviousValue is enabled, and is suppressed for +// tombstones targeting keys with no prior value. +func TestCRDTDeleteHookLoadsPreviousValue(t *testing.T) { t.Parallel() synctest.Test(t, func(t *testing.T) { type hookCall struct { @@ -687,7 +693,8 @@ func TestCRDTOnDelete(t *testing.T) { var mu sync.Mutex opts := DefaultOptions() - opts.OnDelete = func(e DeleteEvent) { + opts.HookLoadPreviousValue = true + opts.DeleteHook = func(e DeleteEvent) { mu.Lock() calls = append(calls, hookCall{e.Key, e.LastValue, e.Delta != nil}) mu.Unlock() @@ -710,7 +717,7 @@ func TestCRDTOnDelete(t *testing.T) { mu.Lock() if len(calls) != len(replicas) { - t.Errorf("expected %d OnDelete calls (one per replica), got %d", len(replicas), len(calls)) + t.Errorf("expected %d DeleteHook calls (one per replica), got %d", len(replicas), len(calls)) } for i, c := range calls { if string(c.lastVal) != "hello" { @@ -734,38 +741,7 @@ func TestCRDTOnDelete(t *testing.T) { mu.Lock() defer mu.Unlock() if n := len(calls); n != 0 { - t.Errorf("OnDelete should not be called for a non-existent key, got %d calls", n) - } - }) -} - -func TestCRDTHooksMutuallyExclusive(t *testing.T) { - t.Parallel() - store := dssync.MutexWrap(ds.NewMapDatastore()) - dagService := mdutils.Mock() - namespace := ds.NewKey("/test") - - t.Run("PutHook and OnPut", func(t *testing.T) { - bcasts, cancelBcasts := newBroadcasters(t, 1) - t.Cleanup(cancelBcasts) - opts := DefaultOptions() - opts.PutHook = func(ds.Key, []byte) {} - opts.OnPut = func(PutEvent) {} - _, err := New(store, namespace, dagService, bcasts[0], opts) - if err == nil { - t.Fatal("expected error when both PutHook and OnPut are set") - } - }) - - t.Run("DeleteHook and OnDelete", func(t *testing.T) { - bcasts, cancelBcasts := newBroadcasters(t, 1) - t.Cleanup(cancelBcasts) - opts := DefaultOptions() - opts.DeleteHook = func(ds.Key) {} - opts.OnDelete = func(DeleteEvent) {} - _, err := New(store, namespace, dagService, bcasts[0], opts) - if err == nil { - t.Fatal("expected error when both DeleteHook and OnDelete are set") + t.Errorf("DeleteHook should not be called for a non-existent key, got %d calls", n) } }) } diff --git a/examples/globaldb/globaldb.go b/examples/globaldb/globaldb.go index 8a30047..910850e 100644 --- a/examples/globaldb/globaldb.go +++ b/examples/globaldb/globaldb.go @@ -193,11 +193,11 @@ func main() { opts := crdt.DefaultOptions() opts.Logger = logger opts.RebroadcastInterval = 5 * time.Second - opts.PutHook = func(k ds.Key, v []byte) { - fmt.Printf("Added: [%s] -> %s\n", k, string(v)) + opts.PutHook = func(e crdt.PutEvent) { + fmt.Printf("Added: [%s] -> %s\n", e.Key, string(e.NewValue)) } - opts.DeleteHook = func(k ds.Key) { - fmt.Printf("Removed: [%s]\n", k) + opts.DeleteHook = func(e crdt.DeleteEvent) { + fmt.Printf("Removed: [%s]\n", e.Key) } crdt, err := crdt.New(store, ds.NewKey("crdt"), ipfs, pubsubBC, opts) diff --git a/set.go b/set.go index f327093..82191b0 100644 --- a/set.go +++ b/set.go @@ -36,13 +36,12 @@ type Set interface { InSet(ctx context.Context, key string) (bool, error) } -// setHooks holds the hook functions for put and delete events. Only one of -// putHook or onPut may be set; likewise for deleteHook and onDelete. +// setHooks holds the hook functions for put and delete events and whether +// the hooks should be invoked with the previous value loaded from the store. type setHooks struct { - putHook func(key ds.Key, v []byte) - onPut func(PutEvent) - deleteHook func(key ds.Key) - onDelete func(DeleteEvent) + putHook func(PutEvent) + deleteHook func(DeleteEvent) + hookLoadPreviousValue bool } // set implements an Add-Wins Observed-Remove Set using delta-CRDTs @@ -85,40 +84,36 @@ func newCRDTSet( return set, nil } -// triggerPutHook calls the appropriate put hook with newVal, oldVal, and the -// triggering delta. oldVal must have been fetched from the store before the -// write. delta is the merged delta that caused the update, or nil when the -// update did not originate from a merged delta (e.g. PurgeDAG). +// triggerPutHook calls the put hook with newVal, oldVal, and the triggering +// delta. oldVal must have been fetched from the store before the write (or be +// nil if hookLoadPreviousValue is false). delta is the merged delta that caused +// the update, or nil when the update did not originate from a merged delta +// (e.g. PurgeDAG). func (s *set) triggerPutHook(key string, newVal, oldVal []byte, delta Delta) { - if s.hooks.onPut != nil { - s.hooks.onPut(PutEvent{ - Key: ds.NewKey(key), - OldValue: oldVal, - NewValue: newVal, - Delta: delta, - }) + if s.hooks.putHook == nil { return } - if s.hooks.putHook != nil { - s.hooks.putHook(ds.NewKey(key), newVal) - } + s.hooks.putHook(PutEvent{ + Key: ds.NewKey(key), + OldValue: oldVal, + NewValue: newVal, + Delta: delta, + }) } -// triggerDeleteHook calls the appropriate delete hook with lastVal (the value -// stored before the deletion) and the triggering tombstone delta, or nil when -// the removal did not originate from a merged delta (e.g. PurgeDAG). +// triggerDeleteHook calls the delete hook with lastVal (the value stored +// before the deletion, or nil if hookLoadPreviousValue is false) and the +// triggering tombstone delta, or nil when the removal did not originate from +// a merged delta (e.g. PurgeDAG). func (s *set) triggerDeleteHook(key string, lastVal []byte, delta Delta) { - if s.hooks.onDelete != nil { - s.hooks.onDelete(DeleteEvent{ - Key: ds.NewKey(key), - LastValue: lastVal, - Delta: delta, - }) + if s.hooks.deleteHook == nil { return } - if s.hooks.deleteHook != nil { - s.hooks.deleteHook(ds.NewKey(key)) - } + s.hooks.deleteHook(DeleteEvent{ + Key: ds.NewKey(key), + LastValue: lastVal, + Delta: delta, + }) } // Add returns a new delta-set adding the given key/value. @@ -408,20 +403,20 @@ func (s *set) setValue(ctx context.Context, writeStore ds.Write, key, id string, var oldVal []byte if prio == curPrio { - oldVal, err = s.store.Get(ctx, valueK) - if err != nil && !errors.Is(err, ds.ErrNotFound) { - s.logger.Errorf("error reading current value for key %s: %s", key, err) - } + oldVal, _ = s.store.Get(ctx, valueK) // new value greater than old if bytes.Compare(oldVal, value) >= 0 { return nil } - } else if s.hooks.onPut != nil { - // Fetch old value before any write for the onPut hook. - oldVal, err = s.store.Get(ctx, valueK) - if err != nil && !errors.Is(err, ds.ErrNotFound) { - s.logger.Errorf("error reading current value for onPut hook: key %s: %s", key, err) - } + } else if s.hooks.hookLoadPreviousValue && s.hooks.putHook != nil { + // Fetch old value before the write so the hook receives it. + oldVal, _ = s.store.Get(ctx, valueK) + } + + // The oldVal read above always happens when prio == curPrio (needed for + // the Compare). Only forward it to the hook when hookLoadPreviousValue is set. + if !s.hooks.hookLoadPreviousValue { + oldVal = nil } // store value @@ -638,7 +633,7 @@ func (s *set) putTombs(ctx context.Context, tombs []*pb.Element, delta Delta) er deletedElems := make(map[string][]string) var newVals, prevVals map[string][]byte - if s.hooks.putHook != nil || s.hooks.onPut != nil || s.hooks.deleteHook != nil || s.hooks.onDelete != nil { + if s.hooks.putHook != nil || s.hooks.deleteHook != nil { // newVals holds the winning value for keys that were partially tombstoned // (tombstone removed a previous winner but a surviving element took over). // A key absent from this map was fully deleted. Doubles as the @@ -646,11 +641,13 @@ func (s *set) putTombs(ctx context.Context, tombs []*pb.Element, delta Delta) er // nil means no hooks are configured and the firing loop is skipped entirely. newVals = make(map[string][]byte) - // prevVals caches the value at the time each key is first seen in this - // delta, before any write. Only keys that existed in the store are added; - // absent keys are omitted so the two-value map lookup (v, ok) cleanly - // distinguishes "had a value" from "was not in the store". - prevVals = make(map[string][]byte) + if s.hooks.hookLoadPreviousValue { + // prevVals caches the value at the time each key is first seen in this + // delta, before any write. Only keys that existed in the store are added; + // absent keys are omitted so the two-value map lookup (v, ok) cleanly + // distinguishes "had a value" from "was not in the store". + prevVals = make(map[string][]byte) + } } var errs []error @@ -664,13 +661,11 @@ func (s *set) putTombs(ctx context.Context, tombs []*pb.Element, delta Delta) er // this key, so hooks receive the pre-tombstone value. if prevVals != nil { if _, seen := prevVals[key]; !seen { - curVal, err := s.store.Get(ctx, valueK) - if err == nil { + if curVal, err := s.store.Get(ctx, valueK); err == nil { prevVals[key] = curVal - } else if !errors.Is(err, ds.ErrNotFound) { - s.logger.Errorf("error reading value before tombstone for key %s: %s", key, err) + // Any error (including ErrNotFound): omit from map; hook + // firing uses (v, ok) to detect absence. } - // ErrNotFound: omit from map; hook firing uses (v, ok) to detect absence. } } deletedElems[key] = append(deletedElems[key], id) @@ -722,18 +717,28 @@ func (s *set) putTombs(ctx context.Context, tombs []*pb.Element, delta Delta) er // Skipped entirely when no hooks are registered (newVals == nil). // Fully deleted keys (absent from newVals) trigger the delete hook; // partially tombstoned keys (present in newVals) trigger the put hook. + // When hookLoadPreviousValue is set, prevVals is used to suppress no-op hook + // firings (value unchanged, or tombstone for a key that had no value). if newVals != nil { for del := range deletedElems { if newVal, partial := newVals[del]; partial { - oldVal := prevVals[del] // nil if key was absent - if bytes.Equal(newVal, oldVal) { - continue // value unchanged, skip hook + var oldVal []byte + if prevVals != nil { + oldVal = prevVals[del] // nil if key was absent + if bytes.Equal(newVal, oldVal) { + // TODO: maybe check priority instead of skipping after byte comparison. + continue // value unchanged, skip hook + } } s.triggerPutHook(del, newVal, oldVal, delta) } else { - lastVal, ok := prevVals[del] - if !ok { - continue // key had no value before tombstone, nothing to notify + var lastVal []byte + if prevVals != nil { + var ok bool + lastVal, ok = prevVals[del] + if !ok { + continue // key had no value before tombstone, nothing to notify + } } s.triggerDeleteHook(del, lastVal, delta) } @@ -887,11 +892,8 @@ func (s *set) purgeKeyBlocks(ctx context.Context, key string, blockCIDs map[cid. // Fetch old value before modifying the value key, so hooks receive the // pre-purge value. Only read when a hook that needs it is configured. var oldVal []byte - if s.hooks.onPut != nil || s.hooks.onDelete != nil { - oldVal, err = s.store.Get(ctx, valueK) - if err != nil && !errors.Is(err, ds.ErrNotFound) { - s.logger.Errorf("error reading value before purge for hook: key %s: %s", key, err) - } + if s.hooks.hookLoadPreviousValue && (s.hooks.putHook != nil || s.hooks.deleteHook != nil) { + oldVal, _ = s.store.Get(ctx, valueK) } if bestVal == nil { diff --git a/set_test.go b/set_test.go index 3b2a771..f4512ce 100644 --- a/set_test.go +++ b/set_test.go @@ -57,8 +57,8 @@ func TestPutTombsEmpty(t *testing.T) { ctx := t.Context() var fired bool s := newTestSet(t, mdutils.Mock(), setHooks{ - putHook: func(ds.Key, []byte) { fired = true }, - deleteHook: func(ds.Key) { fired = true }, + putHook: func(PutEvent) { fired = true }, + deleteHook: func(DeleteEvent) { fired = true }, }) if err := s.putTombs(ctx, nil, nil); err != nil { @@ -103,7 +103,7 @@ func TestPutTombsFullDelete(t *testing.T) { ctx := t.Context() dag := mdutils.Mock() var keys []ds.Key - s := newTestSet(t, dag, setHooks{deleteHook: func(k ds.Key) { keys = append(keys, k) }}) + s := newTestSet(t, dag, setHooks{deleteHook: func(e DeleteEvent) { keys = append(keys, e.Key) }}) id := addElem(t, s, dag, "foo", []byte("hello"), 1) if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id}}, nil); err != nil { @@ -114,14 +114,15 @@ func TestPutTombsFullDelete(t *testing.T) { } }) - t.Run("onDelete receives lastVal", func(t *testing.T) { + t.Run("deleteHook receives lastVal with hookLoadPreviousValue", func(t *testing.T) { t.Parallel() ctx := t.Context() dag := mdutils.Mock() var gotKey ds.Key var gotVal []byte s := newTestSet(t, dag, setHooks{ - onDelete: func(e DeleteEvent) { gotKey, gotVal = e.Key, e.LastValue }, + deleteHook: func(e DeleteEvent) { gotKey, gotVal = e.Key, e.LastValue }, + hookLoadPreviousValue: true, }) id := addElem(t, s, dag, "foo", []byte("hello"), 1) @@ -129,17 +130,17 @@ func TestPutTombsFullDelete(t *testing.T) { t.Fatal(err) } if gotKey.String() != ds.NewKey("foo").String() { - t.Errorf("onDelete key = %q, want /foo", gotKey) + t.Errorf("deleteHook key = %q, want /foo", gotKey) } if string(gotVal) != "hello" { - t.Errorf("onDelete lastVal = %q, want hello", gotVal) + t.Errorf("deleteHook lastVal = %q, want hello", gotVal) } }) } // TestPutTombsNilValue verifies correct hook behavior when a key's stored value // is nil (e.g. Put(ctx, k, nil)). A nil stored value is distinct from "key not -// found": delete hooks must fire, and onDelete must receive nil as lastVal. +// found": delete hooks must fire, and deleteHook must receive nil as lastVal. func TestPutTombsNilValue(t *testing.T) { t.Parallel() @@ -149,7 +150,7 @@ func TestPutTombsNilValue(t *testing.T) { dag := mdutils.Mock() var deleteFired bool s := newTestSet(t, dag, setHooks{ - deleteHook: func(ds.Key) { deleteFired = true }, + deleteHook: func(DeleteEvent) { deleteFired = true }, }) id := addElem(t, s, dag, "foo", nil, 1) @@ -161,7 +162,7 @@ func TestPutTombsNilValue(t *testing.T) { } }) - t.Run("onDelete receives nil lastVal", func(t *testing.T) { + t.Run("deleteHook receives nil lastVal", func(t *testing.T) { t.Parallel() ctx := t.Context() dag := mdutils.Mock() @@ -169,7 +170,8 @@ func TestPutTombsNilValue(t *testing.T) { var gotVal []byte var called bool s := newTestSet(t, dag, setHooks{ - onDelete: func(e DeleteEvent) { gotKey, gotVal, called = e.Key, e.LastValue, true }, + deleteHook: func(e DeleteEvent) { gotKey, gotVal, called = e.Key, e.LastValue, true }, + hookLoadPreviousValue: true, }) id := addElem(t, s, dag, "foo", nil, 1) @@ -177,13 +179,13 @@ func TestPutTombsNilValue(t *testing.T) { t.Fatal(err) } if !called { - t.Fatal("onDelete must be called") + t.Fatal("deleteHook must be called") } if gotKey.String() != ds.NewKey("foo").String() { - t.Errorf("onDelete key = %q, want /foo", gotKey) + t.Errorf("deleteHook key = %q, want /foo", gotKey) } if gotVal != nil { - t.Errorf("onDelete lastVal = %q, want nil", gotVal) + t.Errorf("deleteHook lastVal = %q, want nil", gotVal) } }) @@ -195,8 +197,9 @@ func TestPutTombsNilValue(t *testing.T) { var deleteLastVal []byte var putFired bool s := newTestSet(t, dag, setHooks{ - onPut: func(PutEvent) { putFired = true }, - onDelete: func(e DeleteEvent) { deleteFired = true; deleteLastVal = e.LastValue }, + putHook: func(PutEvent) { putFired = true }, + deleteHook: func(e DeleteEvent) { deleteFired = true; deleteLastVal = e.LastValue }, + hookLoadPreviousValue: true, }) // "aaa" wins over nil lexicographically; store value is "aaa". id1 := addElem(t, s, dag, "foo", nil, 1) // loses @@ -205,11 +208,11 @@ func TestPutTombsNilValue(t *testing.T) { // NOTE: this test documents a known limitation. After tombstoning id2, // id1 (nil value) is still a live, non-tombstoned element — so the key - // should logically remain in the set with a nil value and fire onPut. + // should logically remain in the set with a nil value and fire putHook. // However, findBestValue returns nil for both "no survivors" and // "one survivor whose value is nil", so putTombs cannot distinguish the // two cases. It takes the full-delete path, removes the value key from - // the store, and fires onDelete. As a result InSet returns false even + // the store, and fires deleteHook. As a result InSet returns false even // though a surviving element exists. Fixing this requires findBestValue // to return a separate found bool alongside the value. if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id2}}, nil); err != nil { @@ -217,13 +220,13 @@ func TestPutTombsNilValue(t *testing.T) { } _ = id1 if putFired { - t.Error("onPut must not fire; nil survivor is treated as full delete") + t.Error("putHook must not fire; nil survivor is treated as full delete") } if !deleteFired { - t.Fatal("onDelete must fire when nil survivor is treated as full delete") + t.Fatal("deleteHook must fire when nil survivor is treated as full delete") } if string(deleteLastVal) != "aaa" { - t.Errorf("onDelete lastVal = %q, want aaa", deleteLastVal) + t.Errorf("deleteHook lastVal = %q, want aaa", deleteLastVal) } }) } @@ -261,8 +264,9 @@ func TestPutTombsPartialTombstone(t *testing.T) { dag := mdutils.Mock() var putFired, deleteFired bool s := newTestSet(t, dag, setHooks{ - putHook: func(ds.Key, []byte) { putFired = true }, - deleteHook: func(ds.Key) { deleteFired = true }, + putHook: func(PutEvent) { putFired = true }, + deleteHook: func(DeleteEvent) { deleteFired = true }, + hookLoadPreviousValue: true, }) id1 := addElem(t, s, dag, "foo", []byte("aaa"), 1) // "aaa" loses addElem(t, s, dag, "foo", []byte("bbb"), 1) // "bbb" wins @@ -284,8 +288,8 @@ func TestPutTombsPartialTombstone(t *testing.T) { var putVals [][]byte var deleteFired bool s := newTestSet(t, dag, setHooks{ - putHook: func(_ ds.Key, v []byte) { putVals = append(putVals, v) }, - deleteHook: func(ds.Key) { deleteFired = true }, + putHook: func(e PutEvent) { putVals = append(putVals, e.NewValue) }, + deleteHook: func(DeleteEvent) { deleteFired = true }, }) addElem(t, s, dag, "foo", []byte("aaa"), 1) id2 := addElem(t, s, dag, "foo", []byte("bbb"), 1) // "bbb" wins, is current value @@ -304,32 +308,33 @@ func TestPutTombsPartialTombstone(t *testing.T) { } }) - t.Run("onPut receives newVal and oldVal", func(t *testing.T) { + t.Run("putHook receives newVal and oldVal with hookLoadPreviousValue", func(t *testing.T) { t.Parallel() ctx := t.Context() dag := mdutils.Mock() type call struct{ newVal, oldVal []byte } var calls []call s := newTestSet(t, dag, setHooks{ - onPut: func(e PutEvent) { calls = append(calls, call{e.NewValue, e.OldValue}) }, + putHook: func(e PutEvent) { calls = append(calls, call{e.NewValue, e.OldValue}) }, + hookLoadPreviousValue: true, }) addElem(t, s, dag, "foo", []byte("aaa"), 1) id2 := addElem(t, s, dag, "foo", []byte("bbb"), 1) // "bbb" wins, is current value calls = nil // Tombstone the current winner ("bbb"). "aaa" becomes the new winner: - // onPut must fire with newVal="aaa", oldVal="bbb". + // putHook must fire with newVal="aaa", oldVal="bbb". if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id2}}, nil); err != nil { t.Fatal(err) } if len(calls) != 1 { - t.Fatalf("expected 1 onPut call, got %d", len(calls)) + t.Fatalf("expected 1 putHook call, got %d", len(calls)) } if string(calls[0].newVal) != "aaa" { - t.Errorf("onPut newVal = %q, want aaa", calls[0].newVal) + t.Errorf("putHook newVal = %q, want aaa", calls[0].newVal) } if string(calls[0].oldVal) != "bbb" { - t.Errorf("onPut oldVal = %q, want bbb", calls[0].oldVal) + t.Errorf("putHook oldVal = %q, want bbb", calls[0].oldVal) } }) } @@ -341,21 +346,22 @@ func TestPutTombsMultipleKeys(t *testing.T) { ctx := t.Context() dag := mdutils.Mock() deleted := make(map[string][]byte) - var onPutCalled bool + var putCalled bool s := newTestSet(t, dag, setHooks{ - onDelete: func(e DeleteEvent) { deleted[e.Key.String()] = e.LastValue }, - onPut: func(PutEvent) { onPutCalled = true }, + deleteHook: func(e DeleteEvent) { deleted[e.Key.String()] = e.LastValue }, + putHook: func(PutEvent) { putCalled = true }, + hookLoadPreviousValue: true, }) id1 := addElem(t, s, dag, "key1", []byte("val1"), 1) id2 := addElem(t, s, dag, "key2", []byte("val2"), 1) - onPutCalled = false // discard calls from addElem setup + putCalled = false // discard calls from addElem setup tombs := []*pb.Element{{Key: "key1", Id: id1}, {Key: "key2", Id: id2}} if err := s.putTombs(ctx, tombs, nil); err != nil { t.Fatal(err) } if len(deleted) != 2 { - t.Fatalf("expected 2 onDelete calls, got %d", len(deleted)) + t.Fatalf("expected 2 deleteHook calls, got %d", len(deleted)) } if string(deleted[ds.NewKey("key1").String()]) != "val1" { t.Errorf("key1 lastVal = %q, want val1", deleted[ds.NewKey("key1").String()]) @@ -363,21 +369,23 @@ func TestPutTombsMultipleKeys(t *testing.T) { if string(deleted[ds.NewKey("key2").String()]) != "val2" { t.Errorf("key2 lastVal = %q, want val2", deleted[ds.NewKey("key2").String()]) } - if onPutCalled { - t.Error("onPut must not fire for full-delete tombstones") + if putCalled { + t.Error("putHook must not fire for full-delete tombstones") } } // TestPutTombsNonExistentKey verifies that tombstoning a block ID for a key -// that was never PUT does not crash: the tomb is recorded, and no hooks fire -// since there was no prior value to report. +// that was never PUT does not crash: the tomb is recorded, and when +// hookLoadPreviousValue is set, no hooks fire since there was no prior value to +// report. func TestPutTombsNonExistentKey(t *testing.T) { t.Parallel() ctx := t.Context() var putFired, deleteFired bool s := newTestSet(t, mdutils.Mock(), setHooks{ - onPut: func(PutEvent) { putFired = true }, - onDelete: func(DeleteEvent) { deleteFired = true }, + putHook: func(PutEvent) { putFired = true }, + deleteHook: func(DeleteEvent) { deleteFired = true }, + hookLoadPreviousValue: true, }) if err := s.putTombs(ctx, []*pb.Element{{Key: "ghost", Id: "fakeid"}}, nil); err != nil { @@ -394,16 +402,16 @@ func TestPutTombsNonExistentKey(t *testing.T) { t.Error("key must not be in set") } if putFired { - t.Error("onPut must not fire for a key that was never PUT") + t.Error("putHook must not fire for a key that was never PUT") } if deleteFired { - t.Error("onDelete must not fire when key had no prior value") + t.Error("deleteHook must not fire when key had no prior value") } } // TestPutTombsPrevValsFirstEncounterOnly verifies that prevVals is captured -// before any write for a key: onDelete receives the pre-call value even when -// multiple tombs for the same key appear in one delta. +// before any write for a key: deleteHook receives the pre-call value even +// when multiple tombs for the same key appear in one delta. func TestPutTombsPrevValsFirstEncounterOnly(t *testing.T) { t.Parallel() ctx := t.Context() @@ -411,8 +419,9 @@ func TestPutTombsPrevValsFirstEncounterOnly(t *testing.T) { var putFired bool var lastVals [][]byte s := newTestSet(t, dag, setHooks{ - putHook: func(ds.Key, []byte) { putFired = true }, - onDelete: func(e DeleteEvent) { lastVals = append(lastVals, e.LastValue) }, + putHook: func(PutEvent) { putFired = true }, + deleteHook: func(e DeleteEvent) { lastVals = append(lastVals, e.LastValue) }, + hookLoadPreviousValue: true, }) id1 := addElem(t, s, dag, "foo", []byte("first"), 1) id2 := addElem(t, s, dag, "foo", []byte("second"), 2) // wins (higher prio) @@ -426,10 +435,10 @@ func TestPutTombsPrevValsFirstEncounterOnly(t *testing.T) { t.Error("putHook must not fire when tombstoning a key with a surviving element") } if len(lastVals) != 1 { - t.Fatalf("expected 1 onDelete call, got %d", len(lastVals)) + t.Fatalf("expected 1 deleteHook call, got %d", len(lastVals)) } if string(lastVals[0]) != "second" { - t.Errorf("onDelete lastVal = %q, want second", lastVals[0]) + t.Errorf("deleteHook lastVal = %q, want second", lastVals[0]) } } @@ -440,7 +449,10 @@ func TestPutTombsIdempotent(t *testing.T) { ctx := t.Context() dag := mdutils.Mock() var deleteFiredCount uint8 - s := newTestSet(t, dag, setHooks{deleteHook: func(ds.Key) { deleteFiredCount++ }}) + s := newTestSet(t, dag, setHooks{ + deleteHook: func(DeleteEvent) { deleteFiredCount++ }, + hookLoadPreviousValue: true, + }) id := addElem(t, s, dag, "foo", []byte("hello"), 1) tombs := []*pb.Element{{Key: "foo", Id: id}} @@ -458,13 +470,13 @@ func TestPutTombsIdempotent(t *testing.T) { } // TestPutElemsDeltaForwarded verifies that the Delta passed to putElems is -// forwarded identically to the onPut hook. +// forwarded identically to the putHook. func TestPutElemsDeltaForwarded(t *testing.T) { t.Parallel() ctx := t.Context() var gotDelta Delta s := newTestSet(t, mdutils.Mock(), setHooks{ - onPut: func(e PutEvent) { gotDelta = e.Delta }, + putHook: func(e PutEvent) { gotDelta = e.Delta }, }) var prio uint64 = 42 @@ -473,7 +485,7 @@ func TestPutElemsDeltaForwarded(t *testing.T) { t.Fatal(err) } if gotDelta != d { - t.Fatalf("onPut delta pointer mismatch: got %p want %p", gotDelta, d) + t.Fatalf("putHook delta pointer mismatch: got %p want %p", gotDelta, d) } if gotDelta.GetPriority() != prio { t.Errorf("forwarded delta priority = %d, want %d", gotDelta.GetPriority(), prio) @@ -484,14 +496,14 @@ func TestPutElemsDeltaForwarded(t *testing.T) { } // TestPutTombsFullDeleteDeltaForwarded verifies that a tombstone-only delta is -// forwarded to the onDelete hook when a key is fully deleted. +// forwarded to the deleteHook when a key is fully deleted. func TestPutTombsFullDeleteDeltaForwarded(t *testing.T) { t.Parallel() ctx := t.Context() dag := mdutils.Mock() var gotDelta Delta s := newTestSet(t, dag, setHooks{ - onDelete: func(e DeleteEvent) { gotDelta = e.Delta }, + deleteHook: func(e DeleteEvent) { gotDelta = e.Delta }, }) id := addElem(t, s, dag, "foo", []byte("hello"), 1) @@ -501,7 +513,7 @@ func TestPutTombsFullDeleteDeltaForwarded(t *testing.T) { t.Fatal(err) } if gotDelta != tombDelta { - t.Fatalf("onDelete delta pointer mismatch: got %p want %p", gotDelta, tombDelta) + t.Fatalf("deleteHook delta pointer mismatch: got %p want %p", gotDelta, tombDelta) } if gotDelta.GetDagName() != "tomb-dag" { t.Errorf("forwarded delta dagName = %q, want tomb-dag", gotDelta.GetDagName()) @@ -509,7 +521,7 @@ func TestPutTombsFullDeleteDeltaForwarded(t *testing.T) { } // TestPutTombsPartialPutDeltaForwarded verifies that when a tombstone removes -// the current winner and a surviving element takes over, the onPut hook +// the current winner and a surviving element takes over, the putHook // receives the tombstone delta (the one that triggered the MV change), not // the original put delta. func TestPutTombsPartialPutDeltaForwarded(t *testing.T) { @@ -520,11 +532,12 @@ func TestPutTombsPartialPutDeltaForwarded(t *testing.T) { // Only capture the hook call triggered by putTombs; ignore the addElem puts. var capture bool s := newTestSet(t, dag, setHooks{ - onPut: func(e PutEvent) { + putHook: func(e PutEvent) { if capture { gotEvent = e } }, + hookLoadPreviousValue: true, }) // Seed two elements; "aaa" at prio 2 is the current winner. addElem(t, s, dag, "foo", []byte("zzz"), 1) @@ -532,15 +545,15 @@ func TestPutTombsPartialPutDeltaForwarded(t *testing.T) { capture = true tombDelta := &pbDelta{Delta: &pb.Delta{DagName: "tomb-dag", Priority: 3}} - // Tombstone the winner so "zzz" takes over — that changes the MV and fires onPut. + // Tombstone the winner so "zzz" takes over — that changes the MV and fires putHook. if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id2}}, tombDelta); err != nil { t.Fatal(err) } if gotEvent.Delta != tombDelta { - t.Fatalf("partial-put onPut delta should be the tombstone delta: got %p want %p", gotEvent.Delta, tombDelta) + t.Fatalf("partial-put putHook delta should be the tombstone delta: got %p want %p", gotEvent.Delta, tombDelta) } if string(gotEvent.NewValue) != "zzz" || string(gotEvent.OldValue) != "aaa" { - t.Errorf("onPut values = (new=%q, old=%q), want (new=zzz, old=aaa)", gotEvent.NewValue, gotEvent.OldValue) + t.Errorf("putHook values = (new=%q, old=%q), want (new=zzz, old=aaa)", gotEvent.NewValue, gotEvent.OldValue) } } @@ -551,7 +564,7 @@ func TestCustomDeltaTypeAssert(t *testing.T) { ctx := t.Context() var gotCutsomField int64 s := newTestSet(t, mdutils.Mock(), setHooks{ - onPut: func(e PutEvent) { + putHook: func(e PutEvent) { if cd, ok := e.Delta.(*customDelta); ok { gotCutsomField = cd.CustomField } @@ -601,14 +614,15 @@ func TestPutTombsHigherPriorityWins(t *testing.T) { func TestPurgeKeyBlocksDeltaNil(t *testing.T) { t.Parallel() - t.Run("full purge fires onDelete with nil delta", func(t *testing.T) { + t.Run("full purge fires deleteHook with nil delta", func(t *testing.T) { t.Parallel() ctx := t.Context() dag := mdutils.Mock() var gotEvent DeleteEvent var fired bool s := newTestSet(t, dag, setHooks{ - onDelete: func(e DeleteEvent) { gotEvent = e; fired = true }, + deleteHook: func(e DeleteEvent) { gotEvent = e; fired = true }, + hookLoadPreviousValue: true, }) id := addElem(t, s, dag, "foo", []byte("hello"), 1) @@ -617,24 +631,24 @@ func TestPurgeKeyBlocksDeltaNil(t *testing.T) { t.Fatal(err) } if !fired { - t.Fatal("onDelete should fire for full purge") + t.Fatal("deleteHook should fire for full purge") } if gotEvent.Delta != nil { - t.Errorf("onDelete delta = %v, want nil (no originating delta on purge path)", gotEvent.Delta) + t.Errorf("deleteHook delta = %v, want nil (no originating delta on purge path)", gotEvent.Delta) } if string(gotEvent.LastValue) != "hello" { - t.Errorf("onDelete lastValue = %q, want hello", gotEvent.LastValue) + t.Errorf("deleteHook lastValue = %q, want hello", gotEvent.LastValue) } }) - t.Run("partial purge fires onPut with nil delta", func(t *testing.T) { + t.Run("partial purge fires putHook with nil delta", func(t *testing.T) { t.Parallel() ctx := t.Context() dag := mdutils.Mock() var gotEvent PutEvent var fired bool s := newTestSet(t, dag, setHooks{ - onPut: func(e PutEvent) { gotEvent = e; fired = true }, + putHook: func(e PutEvent) { gotEvent = e; fired = true }, }) id1 := addElem(t, s, dag, "foo", []byte("zzz"), 1) addElem(t, s, dag, "foo", []byte("aaa"), 2) @@ -644,10 +658,10 @@ func TestPurgeKeyBlocksDeltaNil(t *testing.T) { t.Fatal(err) } if !fired { - t.Fatal("onPut should fire when partial purge changes the winner") + t.Fatal("putHook should fire when partial purge changes the winner") } if gotEvent.Delta != nil { - t.Errorf("onPut delta = %v, want nil (no originating delta on purge path)", gotEvent.Delta) + t.Errorf("putHook delta = %v, want nil (no originating delta on purge path)", gotEvent.Delta) } }) } From d1a5aa675e012669f53802c46c4ec8fedf65e94f Mon Sep 17 00:00:00 2001 From: guillaumemichel Date: Tue, 21 Apr 2026 17:55:09 +0200 Subject: [PATCH 05/12] fix: putTombs full delete detection --- set.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/set.go b/set.go index 82191b0..3464d84 100644 --- a/set.go +++ b/set.go @@ -676,7 +676,10 @@ func (s *set) putTombs(ctx context.Context, tombs []*pb.Element, delta Delta) er return err } - if v == nil { + // p == 0 means findBestValue found no surviving element: real deltas + // always have priority >= 1 (assigned as height+1 in addDAGNode), so a + // zero priority can only come from the zero-value init. + if p == 0 { delete(newVals, key) if err = store.Delete(ctx, valueK); err != nil { errs = append(errs, err) @@ -896,7 +899,10 @@ func (s *set) purgeKeyBlocks(ctx context.Context, key string, blockCIDs map[cid. oldVal, _ = s.store.Get(ctx, valueK) } - if bestVal == nil { + // bestPrio == 0 means findBestValue found no surviving element: real deltas + // always have priority >= 1 (assigned as height+1 in addDAGNode), so a zero + // priority can only come from the zero-value init. + if bestPrio == 0 { var errs []error if err := s.store.Delete(ctx, valueK); err != nil && !errors.Is(err, ds.ErrNotFound) { errs = append(errs, err) From cdd58bf130d9d595ca37318b2a24a7a835630e81 Mon Sep 17 00:00:00 2001 From: guillaumemichel Date: Wed, 22 Apr 2026 08:56:26 +0200 Subject: [PATCH 06/12] tests: fix TestPutTombsNilValue --- set_test.go | 49 ++++++++++++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/set_test.go b/set_test.go index f4512ce..257947c 100644 --- a/set_test.go +++ b/set_test.go @@ -189,44 +189,51 @@ func TestPutTombsNilValue(t *testing.T) { } }) - t.Run("nil survivor treated as full delete", func(t *testing.T) { + t.Run("nil survivor keeps key with nil value", func(t *testing.T) { t.Parallel() ctx := t.Context() dag := mdutils.Mock() var deleteFired bool - var deleteLastVal []byte var putFired bool + var putOldVal, putNewVal []byte s := newTestSet(t, dag, setHooks{ - putHook: func(PutEvent) { putFired = true }, - deleteHook: func(e DeleteEvent) { deleteFired = true; deleteLastVal = e.LastValue }, + putHook: func(e PutEvent) { + putFired = true + putOldVal = e.OldValue + putNewVal = e.NewValue + }, + deleteHook: func(DeleteEvent) { deleteFired = true }, hookLoadPreviousValue: true, }) // "aaa" wins over nil lexicographically; store value is "aaa". - id1 := addElem(t, s, dag, "foo", nil, 1) // loses + addElem(t, s, dag, "foo", nil, 1) // loses id2 := addElem(t, s, dag, "foo", []byte("aaa"), 1) // wins putFired = false - // NOTE: this test documents a known limitation. After tombstoning id2, - // id1 (nil value) is still a live, non-tombstoned element — so the key - // should logically remain in the set with a nil value and fire putHook. - // However, findBestValue returns nil for both "no survivors" and - // "one survivor whose value is nil", so putTombs cannot distinguish the - // two cases. It takes the full-delete path, removes the value key from - // the store, and fires deleteHook. As a result InSet returns false even - // though a surviving element exists. Fixing this requires findBestValue - // to return a separate found bool alongside the value. + // After tombstoning id2, id1 (nil value) is still a live, non-tombstoned + // element — the key must remain in the set with a nil value. + // putTombs distinguishes "no survivors" from "nil-valued survivor" via + // priority (p == 0 iff no survivor), so this takes the partial-tombstone + // path: value is replaced with nil and putHook fires. if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id2}}, nil); err != nil { t.Fatal(err) } - _ = id1 - if putFired { - t.Error("putHook must not fire; nil survivor is treated as full delete") + if deleteFired { + t.Error("deleteHook must not fire; a survivor remains") + } + if !putFired { + t.Fatal("putHook must fire; value changed from aaa to nil") } - if !deleteFired { - t.Fatal("deleteHook must fire when nil survivor is treated as full delete") + if string(putOldVal) != "aaa" { + t.Errorf("putHook OldValue = %q, want aaa", putOldVal) + } + if putNewVal != nil { + t.Errorf("putHook NewValue = %q, want nil", putNewVal) } - if string(deleteLastVal) != "aaa" { - t.Errorf("deleteHook lastVal = %q, want aaa", deleteLastVal) + if inSet, err := s.InSet(ctx, "foo"); err != nil { + t.Fatalf("InSet: %v", err) + } else if !inSet { + t.Error("key should remain in set while nil-valued survivor exists") } }) } From 8400cb1b67efdca099d62c5b9e0886970d90ee49 Mon Sep 17 00:00:00 2001 From: guillaumemichel Date: Wed, 22 Apr 2026 09:58:37 +0200 Subject: [PATCH 07/12] feat: pass old and new priorities to hooks --- crdt.go | 43 +++++++++---- set.go | 141 ++++++++++++++++++++++++------------------ set_test.go | 173 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 286 insertions(+), 71 deletions(-) diff --git a/crdt.go b/crdt.go index 9df46a7..7b3f1c9 100644 --- a/crdt.go +++ b/crdt.go @@ -85,6 +85,18 @@ type PutEvent struct { OldValue []byte // NewValue is the new value for the key after the update. NewValue []byte + // OldPriority is the priority of OldValue. It is populated only when + // HookLoadPreviousValue is true, otherwise it is 0. It is also 0 when the + // key had no prior value (real deltas always have priority >= 1, so a zero + // OldPriority means "no previous value"); in that case OldValue is nil as + // well. + OldPriority uint64 + // NewPriority is the priority of NewValue. For normal puts this matches + // Delta.GetPriority(). For partial-tombstone puts (where a tombstone removed + // the previous winner and a surviving element took over), NewPriority is the + // surviving element's priority, which differs from the tombstone delta's + // priority. + NewPriority uint64 // Delta is the triggering delta: the one whose merge caused the state // change. For partial-tombstone puts (where a tombstone removed the previous // winner and a surviving element took over), Delta is the tombstone delta, @@ -101,6 +113,9 @@ type DeleteEvent struct { // LastValue is the previous value for the key, if any. It is populated only // when HookLoadPreviousValue is true, otherwise it is nil. LastValue []byte + // LastPriority is the priority of LastValue. It is populated only when + // HookLoadPreviousValue is true, otherwise it is 0. + LastPriority uint64 // Delta is the tombstone delta that triggered the removal, or nil when the // removal did not originate from a merged delta (e.g. PurgeDAG). Consumers // must not retain Delta past the callback or mutate it. @@ -122,9 +137,10 @@ type Options struct { // (where a tombstone removed the previous winner and a surviving element // took over), the hook fires with the surviving value. Default: nil. // - // PutEvent.OldValue is populated only when HookLoadPreviousValue is true, - // otherwise it is nil. When HookLoadPreviousValue is true, the hook is not - // fired for partial tombstones where the winning value did not change. + // PutEvent.OldValue and PutEvent.OldPriority are populated only when + // HookLoadPreviousValue is true, otherwise they are zero-valued. When + // HookLoadPreviousValue is true, the hook is not fired for partial + // tombstones where the winning value did not change. // // The callback is invoked while internal locks are held; it must not // call back into the Datastore or it will deadlock. @@ -133,21 +149,22 @@ type Options struct { // datastore (either by a local or remote update), i.e. when no surviving // element exists for the key after tombstone processing. Default: nil. // - // DeleteEvent.LastValue is populated only when HookLoadPreviousValue is true, - // otherwise it is nil. When HookLoadPreviousValue is true, the hook is not - // fired for tombstones targeting keys that had no prior value in the - // datastore. + // DeleteEvent.LastValue and DeleteEvent.LastPriority are populated only when + // HookLoadPreviousValue is true, otherwise they are zero-valued. When + // HookLoadPreviousValue is true, the hook is not fired for tombstones + // targeting keys that had no prior value in the datastore. // // The callback is invoked while internal locks are held; it must not // call back into the Datastore or it will deadlock. DeleteHook func(DeleteEvent) // HookLoadPreviousValue controls whether PutHook/DeleteHook receive the - // previous value for the key. When true, the datastore is read before - // each hook-relevant write so that PutEvent.OldValue and - // DeleteEvent.LastValue can be populated; this incurs one extra read per - // triggered hook. When true, the hook is also skipped when the observed - // value would not actually change (see PutHook/DeleteHook doc). Default: - // false. + // previous value and priority for the key. When true, the datastore is read + // before each hook-relevant write so that PutEvent.OldValue / + // PutEvent.OldPriority and DeleteEvent.LastValue / DeleteEvent.LastPriority + // can be populated; this incurs one extra read per triggered hook. When + // true, the hook is also skipped when the observed value would not actually + // change (see PutHook/DeleteHook doc). + // Default: false. HookLoadPreviousValue bool // NumWorkers specifies the number of workers ready to // retrieve and merge deltas while walking the DAGs. Default: diff --git a/set.go b/set.go index 3464d84..555a6ed 100644 --- a/set.go +++ b/set.go @@ -44,6 +44,14 @@ type setHooks struct { hookLoadPreviousValue bool } +// keyState pairs a materialised-view value with its priority. Used inside +// putTombs to snapshot the pre-tombstone state and track the post-tombstone +// winner without maintaining parallel value/priority maps. +type keyState struct { + value []byte + priority uint64 +} + // set implements an Add-Wins Observed-Remove Set using delta-CRDTs // (https://arxiv.org/abs/1410.2803) and backing all the data in a // go-datastore. It is fully agnostic to MerkleCRDTs or the delta distribution @@ -84,35 +92,41 @@ func newCRDTSet( return set, nil } -// triggerPutHook calls the put hook with newVal, oldVal, and the triggering -// delta. oldVal must have been fetched from the store before the write (or be -// nil if hookLoadPreviousValue is false). delta is the merged delta that caused -// the update, or nil when the update did not originate from a merged delta -// (e.g. PurgeDAG). -func (s *set) triggerPutHook(key string, newVal, oldVal []byte, delta Delta) { +// triggerPutHook calls the put hook with newVal, oldVal, their priorities, +// and the triggering delta. oldVal and oldPrio must have been fetched from +// the store before the write (or be zero-valued if hookLoadPreviousValue is +// false). newPrio is the priority of newVal after the write: for normal puts +// this matches delta.GetPriority(); for partial-tombstone puts it is the +// surviving element's priority. delta is the merged delta that caused the +// update, or nil when the update did not originate from a merged delta (e.g. +// PurgeDAG). +func (s *set) triggerPutHook(key string, newVal, oldVal []byte, newPrio, oldPrio uint64, delta Delta) { if s.hooks.putHook == nil { return } s.hooks.putHook(PutEvent{ - Key: ds.NewKey(key), - OldValue: oldVal, - NewValue: newVal, - Delta: delta, + Key: ds.NewKey(key), + OldValue: oldVal, + NewValue: newVal, + OldPriority: oldPrio, + NewPriority: newPrio, + Delta: delta, }) } -// triggerDeleteHook calls the delete hook with lastVal (the value stored -// before the deletion, or nil if hookLoadPreviousValue is false) and the -// triggering tombstone delta, or nil when the removal did not originate from -// a merged delta (e.g. PurgeDAG). -func (s *set) triggerDeleteHook(key string, lastVal []byte, delta Delta) { +// triggerDeleteHook calls the delete hook with lastVal and lastPrio (the +// value and priority stored before the deletion, or zero-valued if +// hookLoadPreviousValue is false) and the triggering tombstone delta, or nil +// when the removal did not originate from a merged delta (e.g. PurgeDAG). +func (s *set) triggerDeleteHook(key string, lastVal []byte, lastPrio uint64, delta Delta) { if s.hooks.deleteHook == nil { return } s.hooks.deleteHook(DeleteEvent{ - Key: ds.NewKey(key), - LastValue: lastVal, - Delta: delta, + Key: ds.NewKey(key), + LastValue: lastVal, + LastPriority: lastPrio, + Delta: delta, }) } @@ -415,8 +429,10 @@ func (s *set) setValue(ctx context.Context, writeStore ds.Write, key, id string, // The oldVal read above always happens when prio == curPrio (needed for // the Compare). Only forward it to the hook when hookLoadPreviousValue is set. + oldPrio := curPrio if !s.hooks.hookLoadPreviousValue { oldVal = nil + oldPrio = 0 } // store value @@ -432,7 +448,7 @@ func (s *set) setValue(ctx context.Context, writeStore ds.Write, key, id string, } // trigger add hook - s.triggerPutHook(key, value, oldVal, delta) + s.triggerPutHook(key, value, oldVal, prio, oldPrio, delta) return nil } @@ -632,21 +648,23 @@ func (s *set) putTombs(ctx context.Context, tombs []*pb.Element, delta Delta) er // element in this delta. deletedElems := make(map[string][]string) - var newVals, prevVals map[string][]byte + var newStates, prevStates map[string]keyState if s.hooks.putHook != nil || s.hooks.deleteHook != nil { - // newVals holds the winning value for keys that were partially tombstoned - // (tombstone removed a previous winner but a surviving element took over). - // A key absent from this map was fully deleted. Doubles as the - // fully-deleted oracle, so it is allocated whenever any hook is registered; - // nil means no hooks are configured and the firing loop is skipped entirely. - newVals = make(map[string][]byte) + // newStates holds the winning (value, priority) for keys that were + // partially tombstoned (tombstone removed a previous winner but a + // surviving element took over). A key absent from this map was fully + // deleted. Doubles as the fully-deleted oracle, so it is allocated + // whenever any hook is registered; nil means no hooks are configured + // and the firing loop is skipped entirely. + newStates = make(map[string]keyState) if s.hooks.hookLoadPreviousValue { - // prevVals caches the value at the time each key is first seen in this - // delta, before any write. Only keys that existed in the store are added; - // absent keys are omitted so the two-value map lookup (v, ok) cleanly - // distinguishes "had a value" from "was not in the store". - prevVals = make(map[string][]byte) + // prevStates caches the (value, priority) at the time each key is + // first seen in this delta, before any write. Only keys that + // existed in the store are added; absent keys are omitted so the + // two-value map lookup (v, ok) cleanly distinguishes "had a value" + // from "was not in the store". + prevStates = make(map[string]keyState) } } @@ -657,14 +675,18 @@ func (s *set) putTombs(ctx context.Context, tombs []*pb.Element, delta Delta) er id := e.GetId() valueK := s.valueKey(key) - // Capture the current value on first encounter, before any write for - // this key, so hooks receive the pre-tombstone value. - if prevVals != nil { - if _, seen := prevVals[key]; !seen { + // Capture the current value and priority on first encounter, before any + // write for this key, so hooks receive the pre-tombstone snapshot. + if prevStates != nil { + if _, seen := prevStates[key]; !seen { if curVal, err := s.store.Get(ctx, valueK); err == nil { - prevVals[key] = curVal + entry := keyState{value: curVal} // Any error (including ErrNotFound): omit from map; hook // firing uses (v, ok) to detect absence. + if curPrio, err := s.getPriority(ctx, key); err == nil { + entry.priority = curPrio + } + prevStates[key] = entry } } } @@ -680,7 +702,7 @@ func (s *set) putTombs(ctx context.Context, tombs []*pb.Element, delta Delta) er // always have priority >= 1 (assigned as height+1 in addDAGNode), so a // zero priority can only come from the zero-value init. if p == 0 { - delete(newVals, key) + delete(newStates, key) if err = store.Delete(ctx, valueK); err != nil { errs = append(errs, err) } @@ -688,8 +710,8 @@ func (s *set) putTombs(ctx context.Context, tombs []*pb.Element, delta Delta) er errs = append(errs, err) } } else { - if newVals != nil { - newVals[key] = v + if newStates != nil { + newStates[key] = keyState{value: v, priority: p} } if err = store.Put(ctx, valueK, v); err != nil { errs = append(errs, err) @@ -717,33 +739,33 @@ func (s *set) putTombs(ctx context.Context, tombs []*pb.Element, delta Delta) er } // Fire hooks once per key after all writes are committed. - // Skipped entirely when no hooks are registered (newVals == nil). - // Fully deleted keys (absent from newVals) trigger the delete hook; - // partially tombstoned keys (present in newVals) trigger the put hook. - // When hookLoadPreviousValue is set, prevVals is used to suppress no-op hook + // Skipped entirely when no hooks are registered (newStates == nil). Fully + // deleted keys (absent from newStates) trigger the delete hook; partially + // tombstoned keys (present in newStates) trigger the put hook. When + // hookLoadPreviousValue is set, prevStates is used to suppress no-op hook // firings (value unchanged, or tombstone for a key that had no value). - if newVals != nil { + if newStates != nil { for del := range deletedElems { - if newVal, partial := newVals[del]; partial { - var oldVal []byte - if prevVals != nil { - oldVal = prevVals[del] // nil if key was absent - if bytes.Equal(newVal, oldVal) { + if newState, partial := newStates[del]; partial { + var prev keyState + if prevStates != nil { + prev = prevStates[del] // zero-valued if key was absent + if bytes.Equal(newState.value, prev.value) { // TODO: maybe check priority instead of skipping after byte comparison. continue // value unchanged, skip hook } } - s.triggerPutHook(del, newVal, oldVal, delta) + s.triggerPutHook(del, newState.value, prev.value, newState.priority, prev.priority, delta) } else { - var lastVal []byte - if prevVals != nil { + var prev keyState + if prevStates != nil { var ok bool - lastVal, ok = prevVals[del] + prev, ok = prevStates[del] if !ok { continue // key had no value before tombstone, nothing to notify } } - s.triggerDeleteHook(del, lastVal, delta) + s.triggerDeleteHook(del, prev.value, prev.priority, delta) } } } @@ -892,11 +914,14 @@ func (s *set) purgeKeyBlocks(ctx context.Context, key string, blockCIDs map[cid. valueK := s.valueKey(key) - // Fetch old value before modifying the value key, so hooks receive the - // pre-purge value. Only read when a hook that needs it is configured. + // Fetch old value and priority before modifying the value key, so hooks + // receive the pre-purge snapshot. Only read when a hook that needs it is + // configured. var oldVal []byte + var oldPrio uint64 if s.hooks.hookLoadPreviousValue && (s.hooks.putHook != nil || s.hooks.deleteHook != nil) { oldVal, _ = s.store.Get(ctx, valueK) + oldPrio, _ = s.getPriority(ctx, key) } // bestPrio == 0 means findBestValue found no surviving element: real deltas @@ -913,7 +938,7 @@ func (s *set) purgeKeyBlocks(ctx context.Context, key string, blockCIDs map[cid. if err := errors.Join(errs...); err != nil { return err } - s.triggerDeleteHook(key, oldVal, nil) + s.triggerDeleteHook(key, oldVal, oldPrio, nil) } else { if err := s.store.Put(ctx, valueK, bestVal); err != nil { return err @@ -921,7 +946,7 @@ func (s *set) purgeKeyBlocks(ctx context.Context, key string, blockCIDs map[cid. if err := s.setPriority(ctx, s.store, key, bestPrio); err != nil { return err } - s.triggerPutHook(key, bestVal, oldVal, nil) + s.triggerPutHook(key, bestVal, oldVal, bestPrio, oldPrio, nil) } return nil diff --git a/set_test.go b/set_test.go index 257947c..2eff351 100644 --- a/set_test.go +++ b/set_test.go @@ -564,6 +564,129 @@ func TestPutTombsPartialPutDeltaForwarded(t *testing.T) { } } +// TestPutElemsPrioritiesReported verifies that PutEvent.NewPriority matches +// the delta priority for a normal put and PutEvent.OldPriority carries the +// replaced value's priority when HookLoadPreviousValue is set. +func TestPutElemsPrioritiesReported(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + var gotEvent PutEvent + capture := false + s := newTestSet(t, dag, setHooks{ + putHook: func(e PutEvent) { + if capture { + gotEvent = e + } + }, + hookLoadPreviousValue: true, + }) + var oldPrio uint64 = 3 + addElem(t, s, dag, "foo", []byte("old"), oldPrio) + + capture = true + var newPrio uint64 = 7 + d := &pbDelta{Delta: &pb.Delta{Priority: newPrio}} + if err := s.putElems(ctx, []*pb.Element{{Key: "foo", Value: []byte("new")}}, "block-new", d); err != nil { + t.Fatal(err) + } + if gotEvent.NewPriority != newPrio { + t.Errorf("NewPriority = %d, want %d (delta priority)", gotEvent.NewPriority, newPrio) + } + if gotEvent.OldPriority != oldPrio { + t.Errorf("OldPriority = %d, want %d (prior value's priority)", gotEvent.OldPriority, oldPrio) + } +} + +// TestPutElemsOldPriorityZeroWhenNotLoaded verifies that OldPriority is 0 +// when HookLoadPreviousValue is false, matching the OldValue behavior. +func TestPutElemsOldPriorityZeroWhenNotLoaded(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + var gotEvent PutEvent + capture := false + s := newTestSet(t, dag, setHooks{ + putHook: func(e PutEvent) { + if capture { + gotEvent = e + } + }, + }) + addElem(t, s, dag, "foo", []byte("old"), 3) + + capture = true + var newPrio uint64 = 7 + d := &pbDelta{Delta: &pb.Delta{Priority: newPrio}} + if err := s.putElems(ctx, []*pb.Element{{Key: "foo", Value: []byte("new")}}, "block-new", d); err != nil { + t.Fatal(err) + } + if gotEvent.NewPriority != newPrio { + t.Errorf("NewPriority = %d, want %d", gotEvent.NewPriority, newPrio) + } + if gotEvent.OldPriority != 0 { + t.Errorf("OldPriority = %d, want 0 (HookLoadPreviousValue is false)", gotEvent.OldPriority) + } +} + +// TestPutTombsPartialPutPriorities verifies that for a partial-tombstone put +// NewPriority carries the surviving element's priority (not the tombstone +// delta's priority) and OldPriority carries the previous winner's priority. +// This is the case where NewPriority cannot be inferred from Delta. +func TestPutTombsPartialPutPriorities(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + var gotEvent PutEvent + capture := false + s := newTestSet(t, dag, setHooks{ + putHook: func(e PutEvent) { + if capture { + gotEvent = e + } + }, + hookLoadPreviousValue: true, + }) + // Seed two elements; "aaa" at prio 2 is the current winner. + addElem(t, s, dag, "foo", []byte("zzz"), 1) + id2 := addElem(t, s, dag, "foo", []byte("aaa"), 2) + + capture = true + // Tombstone delta priority (5) is deliberately different from the + // surviving element priority (1) to prove they are distinct. + tombDelta := &pbDelta{Delta: &pb.Delta{Priority: 5}} + if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id2}}, tombDelta); err != nil { + t.Fatal(err) + } + if gotEvent.NewPriority != 1 { + t.Errorf("NewPriority = %d, want 1 (surviving element's priority, not the tombstone delta's)", gotEvent.NewPriority) + } + if gotEvent.OldPriority != 2 { + t.Errorf("OldPriority = %d, want 2 (tombstoned winner's priority)", gotEvent.OldPriority) + } +} + +// TestPutTombsFullDeleteLastPriority verifies that the DeleteEvent carries +// the priority of the removed value when HookLoadPreviousValue is set. +func TestPutTombsFullDeleteLastPriority(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + var gotEvent DeleteEvent + s := newTestSet(t, dag, setHooks{ + deleteHook: func(e DeleteEvent) { gotEvent = e }, + hookLoadPreviousValue: true, + }) + id := addElem(t, s, dag, "foo", []byte("hello"), 4) + + if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id}}, &pbDelta{Delta: &pb.Delta{Priority: 9}}); err != nil { + t.Fatal(err) + } + if gotEvent.LastPriority != 4 { + t.Errorf("LastPriority = %d, want 4 (deleted value's priority)", gotEvent.LastPriority) + } +} + // TestCustomDeltaTypeAssert verifies that callbacks can type-assert the Delta // back to a concrete implementation to reach application-specific fields func TestCustomDeltaTypeAssert(t *testing.T) { @@ -673,6 +796,56 @@ func TestPurgeKeyBlocksDeltaNil(t *testing.T) { }) } +// TestPurgeKeyBlocksPriorities verifies that hooks fired from the purge path +// carry the correct old/new priorities when HookLoadPreviousValue is set. +func TestPurgeKeyBlocksPriorities(t *testing.T) { + t.Parallel() + + t.Run("full purge reports LastPriority", func(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + var gotEvent DeleteEvent + s := newTestSet(t, dag, setHooks{ + deleteHook: func(e DeleteEvent) { gotEvent = e }, + hookLoadPreviousValue: true, + }) + id := addElem(t, s, dag, "foo", []byte("hello"), 6) + + c := blockIDToCid(t, id) + if err := s.purgeKeyBlocks(ctx, "foo", map[cid.Cid]struct{}{c: {}}, true, false); err != nil { + t.Fatal(err) + } + if gotEvent.LastPriority != 6 { + t.Errorf("LastPriority = %d, want 6", gotEvent.LastPriority) + } + }) + + t.Run("partial purge reports old and new priorities", func(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + var gotEvent PutEvent + s := newTestSet(t, dag, setHooks{ + putHook: func(e PutEvent) { gotEvent = e }, + hookLoadPreviousValue: true, + }) + addElem(t, s, dag, "foo", []byte("zzz"), 1) + id2 := addElem(t, s, dag, "foo", []byte("aaa"), 2) // current winner + + c2 := blockIDToCid(t, id2) + if err := s.purgeKeyBlocks(ctx, "foo", map[cid.Cid]struct{}{c2: {}}, true, false); err != nil { + t.Fatal(err) + } + if gotEvent.OldPriority != 2 { + t.Errorf("OldPriority = %d, want 2 (purged winner's priority)", gotEvent.OldPriority) + } + if gotEvent.NewPriority != 1 { + t.Errorf("NewPriority = %d, want 1 (surviving element's priority)", gotEvent.NewPriority) + } + }) +} + // blockIDToCid converts the blockKey returned by addElem (a datastore-keyed // multihash) back into a CID, for use with purgeKeyBlocks. func blockIDToCid(t *testing.T, blockID string) cid.Cid { From bcdc85709a7c4ae1032f6663bcecf050787c176c Mon Sep 17 00:00:00 2001 From: guillaumemichel Date: Wed, 22 Apr 2026 10:19:38 +0200 Subject: [PATCH 08/12] fix: don't fire delete hook when purging non-existing key --- set.go | 21 +++++++++++------ set_test.go | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 7 deletions(-) diff --git a/set.go b/set.go index 555a6ed..2d21a9d 100644 --- a/set.go +++ b/set.go @@ -916,12 +916,12 @@ func (s *set) purgeKeyBlocks(ctx context.Context, key string, blockCIDs map[cid. // Fetch old value and priority before modifying the value key, so hooks // receive the pre-purge snapshot. Only read when a hook that needs it is - // configured. - var oldVal []byte - var oldPrio uint64 + // configured. A missing key yields prev.value == nil (Get returns nil on + // ErrNotFound), which is used below to detect "no prior value". + var prev keyState if s.hooks.hookLoadPreviousValue && (s.hooks.putHook != nil || s.hooks.deleteHook != nil) { - oldVal, _ = s.store.Get(ctx, valueK) - oldPrio, _ = s.getPriority(ctx, key) + prev.value, _ = s.store.Get(ctx, valueK) + prev.priority, _ = s.getPriority(ctx, key) } // bestPrio == 0 means findBestValue found no surviving element: real deltas @@ -938,7 +938,14 @@ func (s *set) purgeKeyBlocks(ctx context.Context, key string, blockCIDs map[cid. if err := errors.Join(errs...); err != nil { return err } - s.triggerDeleteHook(key, oldVal, oldPrio, nil) + // Suppress the delete hook when the key had no prior value in the + // store. Matches putTombs' "nothing to notify" rule. Only active when + // hookLoadPreviousValue is set, since that's when we know the prior + // state. + if s.hooks.hookLoadPreviousValue && prev.value == nil { + return nil + } + s.triggerDeleteHook(key, prev.value, prev.priority, nil) } else { if err := s.store.Put(ctx, valueK, bestVal); err != nil { return err @@ -946,7 +953,7 @@ func (s *set) purgeKeyBlocks(ctx context.Context, key string, blockCIDs map[cid. if err := s.setPriority(ctx, s.store, key, bestPrio); err != nil { return err } - s.triggerPutHook(key, bestVal, oldVal, bestPrio, oldPrio, nil) + s.triggerPutHook(key, bestVal, prev.value, bestPrio, prev.priority, nil) } return nil diff --git a/set_test.go b/set_test.go index 2eff351..e759cda 100644 --- a/set_test.go +++ b/set_test.go @@ -846,6 +846,73 @@ func TestPurgeKeyBlocksPriorities(t *testing.T) { }) } +// TestPurgeKeyBlocksSuppression covers the two edge cases where purgeKeyBlocks +// needs to make a decision about whether to fire a hook: +// 1. full purge on a key that had no prior value → deleteHook must NOT fire +// (nothing to notify — there was no value to delete) +// 2. partial purge where the surviving value equals the pre-purge value → +// putHook must STILL fire, because a partial purge always replaces the +// winning element and therefore changes the priority +func TestPurgeKeyBlocksSuppression(t *testing.T) { + t.Parallel() + + t.Run("full purge on key with no prior value suppresses deleteHook", func(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + var deleteFired bool + s := newTestSet(t, dag, setHooks{ + deleteHook: func(DeleteEvent) { deleteFired = true }, + hookLoadPreviousValue: true, + }) + // Create a real block for an unrelated key so we have a valid CID, but + // never insert an element under "ghost": the value key for "ghost" is + // absent, so hadPrior must be false. + blockID := addElem(t, s, dag, "other", []byte("x"), 1) + c := blockIDToCid(t, blockID) + + if err := s.purgeKeyBlocks(ctx, "ghost", map[cid.Cid]struct{}{c: {}}, true, false); err != nil { + t.Fatal(err) + } + if deleteFired { + t.Error("deleteHook must not fire when purged key had no prior value") + } + }) + + t.Run("partial purge with unchanged value still fires putHook (priority changed)", func(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + var gotEvent PutEvent + var fired bool + s := newTestSet(t, dag, setHooks{ + putHook: func(e PutEvent) { gotEvent = e; fired = true }, + hookLoadPreviousValue: true, + }) + // Two elements with the SAME value at different priorities. The higher + // priority (id2) currently wins. Purging id2 leaves id1 — identical + // value, but lower priority. The priority change IS a state transition + // and must be surfaced via the put hook. + addElem(t, s, dag, "foo", []byte("same"), 1) + id2 := addElem(t, s, dag, "foo", []byte("same"), 2) + fired = false // reset: the second addElem fires putHook + + c := blockIDToCid(t, id2) + if err := s.purgeKeyBlocks(ctx, "foo", map[cid.Cid]struct{}{c: {}}, true, false); err != nil { + t.Fatal(err) + } + if !fired { + t.Fatal("putHook must fire on partial purge, even when value is unchanged, to surface the priority change") + } + if gotEvent.OldPriority != 2 { + t.Errorf("OldPriority = %d, want 2", gotEvent.OldPriority) + } + if gotEvent.NewPriority != 1 { + t.Errorf("NewPriority = %d, want 1", gotEvent.NewPriority) + } + }) +} + // blockIDToCid converts the blockKey returned by addElem (a datastore-keyed // multihash) back into a CID, for use with purgeKeyBlocks. func blockIDToCid(t *testing.T, blockID string) cid.Cid { From f6b6539659de8a0c5dbbc0bfedb86aa723fa64a7 Mon Sep 17 00:00:00 2001 From: guillaumemichel Date: Wed, 22 Apr 2026 10:34:38 +0200 Subject: [PATCH 09/12] fix: fire put hook in putTombs if new winner has same value but different priority --- crdt.go | 3 ++- set.go | 8 ++++---- set_test.go | 43 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 5 deletions(-) diff --git a/crdt.go b/crdt.go index 7b3f1c9..0d3efc1 100644 --- a/crdt.go +++ b/crdt.go @@ -140,7 +140,8 @@ type Options struct { // PutEvent.OldValue and PutEvent.OldPriority are populated only when // HookLoadPreviousValue is true, otherwise they are zero-valued. When // HookLoadPreviousValue is true, the hook is not fired for partial - // tombstones where the winning value did not change. + // tombstones where the winning element is unchanged (same value and + // priority); a same-value/different-priority swap still fires the hook. // // The callback is invoked while internal locks are held; it must not // call back into the Datastore or it will deadlock. diff --git a/set.go b/set.go index 2d21a9d..423dc43 100644 --- a/set.go +++ b/set.go @@ -743,16 +743,16 @@ func (s *set) putTombs(ctx context.Context, tombs []*pb.Element, delta Delta) er // deleted keys (absent from newStates) trigger the delete hook; partially // tombstoned keys (present in newStates) trigger the put hook. When // hookLoadPreviousValue is set, prevStates is used to suppress no-op hook - // firings (value unchanged, or tombstone for a key that had no value). + // firings (winning element unchanged — same value and priority — or + // tombstone for a key that had no value). if newStates != nil { for del := range deletedElems { if newState, partial := newStates[del]; partial { var prev keyState if prevStates != nil { prev = prevStates[del] // zero-valued if key was absent - if bytes.Equal(newState.value, prev.value) { - // TODO: maybe check priority instead of skipping after byte comparison. - continue // value unchanged, skip hook + if newState.priority == prev.priority && bytes.Equal(newState.value, prev.value) { + continue // same winning element, genuine no-op } } s.triggerPutHook(del, newState.value, prev.value, newState.priority, prev.priority, delta) diff --git a/set_test.go b/set_test.go index e759cda..35d2bb2 100644 --- a/set_test.go +++ b/set_test.go @@ -1,6 +1,7 @@ package crdt import ( + "bytes" "testing" dshelp "github.com/ipfs/boxo/datastore/dshelp" @@ -666,6 +667,48 @@ func TestPutTombsPartialPutPriorities(t *testing.T) { } } +// TestPutTombsPartialPutSameValueDifferentPriority verifies that when a +// tombstone removes the current winner and a surviving element with the SAME +// value bytes but a different priority takes over, the putHook still fires — +// the priority change is a state transition that must be surfaced even though +// the value bytes are unchanged. +func TestPutTombsPartialPutSameValueDifferentPriority(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + var gotEvent PutEvent + var fired bool + s := newTestSet(t, dag, setHooks{ + putHook: func(e PutEvent) { gotEvent = e; fired = true }, + hookLoadPreviousValue: true, + }) + // Two elements with the SAME value at different priorities. The higher + // priority (id2) currently wins. Tombstoning id2 leaves id1 — identical + // value, but lower priority. + addElem(t, s, dag, "foo", []byte("same"), 1) + id2 := addElem(t, s, dag, "foo", []byte("same"), 2) + fired = false // reset: the second addElem fires putHook + + if err := s.putTombs(ctx, []*pb.Element{{Key: "foo", Id: id2}}, &pbDelta{Delta: &pb.Delta{Priority: 5}}); err != nil { + t.Fatal(err) + } + if !fired { + t.Fatal("putHook must fire on partial tombstone, even when value is unchanged, to surface the priority change") + } + if gotEvent.OldPriority != 2 { + t.Errorf("OldPriority = %d, want 2", gotEvent.OldPriority) + } + if gotEvent.NewPriority != 1 { + t.Errorf("NewPriority = %d, want 1", gotEvent.NewPriority) + } + if !bytes.Equal(gotEvent.OldValue, []byte("same")) { + t.Errorf("OldValue = %q, want same", gotEvent.OldValue) + } + if !bytes.Equal(gotEvent.NewValue, []byte("same")) { + t.Errorf("NewValue = %q, want same", gotEvent.NewValue) + } +} + // TestPutTombsFullDeleteLastPriority verifies that the DeleteEvent carries // the priority of the removed value when HookLoadPreviousValue is set. func TestPutTombsFullDeleteLastPriority(t *testing.T) { From f45c196a84f3feefc84a5e8d35e64e3f58ced1ec Mon Sep 17 00:00:00 2001 From: guillaumemichel Date: Fri, 24 Apr 2026 09:12:06 +0200 Subject: [PATCH 10/12] fix: fire putElems hooks after batch commit --- set.go | 123 ++++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 79 insertions(+), 44 deletions(-) diff --git a/set.go b/set.go index 423dc43..603cf8b 100644 --- a/set.go +++ b/set.go @@ -92,42 +92,25 @@ func newCRDTSet( return set, nil } -// triggerPutHook calls the put hook with newVal, oldVal, their priorities, -// and the triggering delta. oldVal and oldPrio must have been fetched from -// the store before the write (or be zero-valued if hookLoadPreviousValue is -// false). newPrio is the priority of newVal after the write: for normal puts -// this matches delta.GetPriority(); for partial-tombstone puts it is the -// surviving element's priority. delta is the merged delta that caused the -// update, or nil when the update did not originate from a merged delta (e.g. -// PurgeDAG). -func (s *set) triggerPutHook(key string, newVal, oldVal []byte, newPrio, oldPrio uint64, delta Delta) { +// triggerPutHook fires the put hook with evt. Callers build evt themselves +// and must invoke this after the relevant batch has been committed, so the +// hook callback can observe the post-write state via s.store. +func (s *set) triggerPutHook(evt PutEvent) { if s.hooks.putHook == nil { return } - s.hooks.putHook(PutEvent{ - Key: ds.NewKey(key), - OldValue: oldVal, - NewValue: newVal, - OldPriority: oldPrio, - NewPriority: newPrio, - Delta: delta, - }) + s.hooks.putHook(evt) } -// triggerDeleteHook calls the delete hook with lastVal and lastPrio (the -// value and priority stored before the deletion, or zero-valued if -// hookLoadPreviousValue is false) and the triggering tombstone delta, or nil -// when the removal did not originate from a merged delta (e.g. PurgeDAG). -func (s *set) triggerDeleteHook(key string, lastVal []byte, lastPrio uint64, delta Delta) { +// triggerDeleteHook fires the delete hook with evt. Callers build evt +// themselves and must invoke this after the relevant batch has been +// committed, so the hook callback can observe the post-write state via +// s.store. +func (s *set) triggerDeleteHook(evt DeleteEvent) { if s.hooks.deleteHook == nil { return } - s.hooks.deleteHook(DeleteEvent{ - Key: ds.NewKey(key), - LastValue: lastVal, - LastPriority: lastPrio, - Delta: delta, - }) + s.hooks.deleteHook(evt) } // Add returns a new delta-set adding the given key/value. @@ -394,24 +377,30 @@ func (s *set) setPriority(ctx context.Context, writeStore ds.Write, key string, // delta is the triggering delta forwarded to the put hook when the write is // considered the prevalent value. delta must not be nil; its priority drives // the write decision. -func (s *set) setValue(ctx context.Context, writeStore ds.Write, key, id string, value []byte, delta Delta) error { +// setValue writes value at the given priority for key when the delta wins +// (higher priority, or equal priority with a greater byte value). It returns +// a non-nil *PutEvent describing the change when a put hook is registered +// and the write actually happened; otherwise it returns (nil, nil). The +// caller is responsible for firing the hook (typically after the surrounding +// batch is committed, so the hook callback observes post-write state). +func (s *set) setValue(ctx context.Context, writeStore ds.Write, key, id string, value []byte, delta Delta) (*PutEvent, error) { if delta == nil { - return errors.New("setValue: delta must not be nil") + return nil, errors.New("setValue: delta must not be nil") } // If this key was tombstoned already, do not store/update the value. deleted, err := s.inTombsKeyID(ctx, key, id) if err != nil || deleted { - return err + return nil, err } prio := delta.GetPriority() curPrio, err := s.getPriority(ctx, key) if err != nil { - return err + return nil, err } if prio < curPrio { - return nil + return nil, nil } valueK := s.valueKey(key) @@ -420,7 +409,7 @@ func (s *set) setValue(ctx context.Context, writeStore ds.Write, key, id string, oldVal, _ = s.store.Get(ctx, valueK) // new value greater than old if bytes.Compare(oldVal, value) >= 0 { - return nil + return nil, nil } } else if s.hooks.hookLoadPreviousValue && s.hooks.putHook != nil { // Fetch old value before the write so the hook receives it. @@ -438,18 +427,26 @@ func (s *set) setValue(ctx context.Context, writeStore ds.Write, key, id string, // store value err = writeStore.Put(ctx, valueK, value) if err != nil { - return err + return nil, err } // store priority err = s.setPriority(ctx, writeStore, key, prio) if err != nil { - return err + return nil, err } - // trigger add hook - s.triggerPutHook(key, value, oldVal, prio, oldPrio, delta) - return nil + if s.hooks.putHook == nil { + return nil, nil + } + return &PutEvent{ + Key: ds.NewKey(key), + OldValue: oldVal, + NewValue: value, + OldPriority: oldPrio, + NewPriority: prio, + Delta: delta, + }, nil } // findBestValue looks for all entries for the given key, figures out their @@ -597,6 +594,15 @@ func (s *set) putElems(ctx context.Context, elems []*pb.Element, id string, delt } } + // Collect put events during the loop and fire the hooks after the batch + // commit so callbacks reading from s.store observe the post-merge + // snapshot. Mirrors putTombs. nil when no put hook is registered, so the + // firing loop becomes a no-op. + var events []PutEvent + if s.hooks.putHook != nil { + events = make([]PutEvent, 0, len(elems)) + } + for _, e := range elems { e.Id = id // overwrite the identifier as it would come unset key := e.GetKey() @@ -610,10 +616,13 @@ func (s *set) putElems(ctx context.Context, elems []*pb.Element, id string, delt // update the value if applicable: // * higher priority than we currently have. // * not tombstoned before. - err = s.setValue(ctx, store, key, id, e.GetValue(), delta) + evt, err := s.setValue(ctx, store, key, id, e.GetValue(), delta) if err != nil { return err } + if evt != nil { + events = append(events, *evt) + } } if batching { @@ -622,6 +631,10 @@ func (s *set) putElems(ctx context.Context, elems []*pb.Element, id string, delt return err } } + + for _, evt := range events { + s.triggerPutHook(evt) + } return nil } @@ -755,7 +768,14 @@ func (s *set) putTombs(ctx context.Context, tombs []*pb.Element, delta Delta) er continue // same winning element, genuine no-op } } - s.triggerPutHook(del, newState.value, prev.value, newState.priority, prev.priority, delta) + s.triggerPutHook(PutEvent{ + Key: ds.NewKey(del), + OldValue: prev.value, + NewValue: newState.value, + OldPriority: prev.priority, + NewPriority: newState.priority, + Delta: delta, + }) } else { var prev keyState if prevStates != nil { @@ -765,7 +785,12 @@ func (s *set) putTombs(ctx context.Context, tombs []*pb.Element, delta Delta) er continue // key had no value before tombstone, nothing to notify } } - s.triggerDeleteHook(del, prev.value, prev.priority, delta) + s.triggerDeleteHook(DeleteEvent{ + Key: ds.NewKey(del), + LastValue: prev.value, + LastPriority: prev.priority, + Delta: delta, + }) } } } @@ -945,7 +970,11 @@ func (s *set) purgeKeyBlocks(ctx context.Context, key string, blockCIDs map[cid. if s.hooks.hookLoadPreviousValue && prev.value == nil { return nil } - s.triggerDeleteHook(key, prev.value, prev.priority, nil) + s.triggerDeleteHook(DeleteEvent{ + Key: ds.NewKey(key), + LastValue: prev.value, + LastPriority: prev.priority, + }) } else { if err := s.store.Put(ctx, valueK, bestVal); err != nil { return err @@ -953,7 +982,13 @@ func (s *set) purgeKeyBlocks(ctx context.Context, key string, blockCIDs map[cid. if err := s.setPriority(ctx, s.store, key, bestPrio); err != nil { return err } - s.triggerPutHook(key, bestVal, prev.value, bestPrio, prev.priority, nil) + s.triggerPutHook(PutEvent{ + Key: ds.NewKey(key), + OldValue: prev.value, + NewValue: bestVal, + OldPriority: prev.priority, + NewPriority: bestPrio, + }) } return nil From 56a4fcfe13beb57031c467676ac078cc019f9b64 Mon Sep 17 00:00:00 2001 From: guillaumemichel Date: Mon, 27 Apr 2026 17:52:28 +0200 Subject: [PATCH 11/12] tests: add failing test for putElems duplicate-key dedup --- set_test.go | 146 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 146 insertions(+) diff --git a/set_test.go b/set_test.go index 35d2bb2..027c84d 100644 --- a/set_test.go +++ b/set_test.go @@ -782,6 +782,152 @@ func TestPutTombsHigherPriorityWins(t *testing.T) { } } +// TestCRDTPutElemsDuplicateKeyInSameDelta verifies that when a single delta +// contains multiple elements for the same key, putElems treats the group as +// one logical key transition: +// +// 1. The put hook fires exactly once per key, not once per element. Firing +// per-element makes downstream observers (e.g. usage accountants) count a +// single key insertion N times. +// 2. The winning value follows CRDT conflict resolution across the elements +// — all share the delta's priority, so the lex-largest value wins — +// independent of the elements' order in the slice. +// 3. The PutEvent reports the pre-delta store snapshot as OldValue, not a +// sibling element's value from earlier in the same iteration. +// +// The test mirrors putTombs' per-key deduplication pattern (see +// TestPutTombsMultipleKeys and the newStates/prevStates handling in +// putTombs) and is expected to fail against the current putElems +// implementation, which iterates elements independently and emits one +// PutEvent per element. +func TestCRDTPutElemsDuplicateKeyInSameDelta(t *testing.T) { + t.Parallel() + + t.Run("hook fires once per key", func(t *testing.T) { + t.Parallel() + ctx := t.Context() + var events []PutEvent + s := newTestSet(t, mdutils.Mock(), setHooks{ + putHook: func(e PutEvent) { events = append(events, e) }, + hookLoadPreviousValue: true, + }) + + d := &pbDelta{Delta: &pb.Delta{Priority: 1}} + elems := []*pb.Element{ + {Key: "foo", Value: []byte("zzz")}, + {Key: "foo", Value: []byte("aaa")}, + } + if err := s.putElems(ctx, elems, "block-multi", d); err != nil { + t.Fatal(err) + } + if len(events) != 1 { + t.Fatalf("putHook call count = %d, want 1 (one hook per key, not per element)", len(events)) + } + }) + + t.Run("winning value is lex-largest regardless of order", func(t *testing.T) { + t.Parallel() + + // Element ordering within the delta must not affect the outcome: + // conflict resolution across same-key, same-priority elements picks + // the lex-largest value. + for _, tc := range []struct { + name string + elems []*pb.Element + }{ + { + name: "ascending (aaa then zzz)", + elems: []*pb.Element{ + {Key: "foo", Value: []byte("aaa")}, + {Key: "foo", Value: []byte("zzz")}, + }, + }, + { + name: "descending (zzz then aaa)", + elems: []*pb.Element{ + {Key: "foo", Value: []byte("zzz")}, + {Key: "foo", Value: []byte("aaa")}, + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := t.Context() + var events []PutEvent + s := newTestSet(t, mdutils.Mock(), setHooks{ + putHook: func(e PutEvent) { events = append(events, e) }, + hookLoadPreviousValue: true, + }) + + d := &pbDelta{Delta: &pb.Delta{Priority: 1}} + if err := s.putElems(ctx, tc.elems, "block-multi", d); err != nil { + t.Fatal(err) + } + if len(events) != 1 { + t.Fatalf("putHook call count = %d, want 1", len(events)) + } + if !bytes.Equal(events[0].NewValue, []byte("zzz")) { + t.Errorf("NewValue = %q, want zzz (lex-largest at tied priority)", events[0].NewValue) + } + if events[0].OldValue != nil { + t.Errorf("OldValue = %q, want nil (key did not exist pre-delta)", events[0].OldValue) + } + val, err := s.Element(ctx, "foo") + if err != nil { + t.Fatalf("Element: %v", err) + } + if !bytes.Equal(val, []byte("zzz")) { + t.Errorf("stored value = %q, want zzz (CRDT winner must not depend on element order in delta)", val) + } + }) + } + }) + + t.Run("OldValue is pre-delta snapshot, not a sibling in-delta value", func(t *testing.T) { + t.Parallel() + ctx := t.Context() + dag := mdutils.Mock() + var events []PutEvent + capture := false + s := newTestSet(t, dag, setHooks{ + putHook: func(e PutEvent) { + if capture { + events = append(events, e) + } + }, + hookLoadPreviousValue: true, + }) + + // Seed the store so the key has a committed pre-delta value. + addElem(t, s, dag, "foo", []byte("seed"), 1) + + capture = true + d := &pbDelta{Delta: &pb.Delta{Priority: 2}} + elems := []*pb.Element{ + {Key: "foo", Value: []byte("a")}, + {Key: "foo", Value: []byte("z")}, // in-delta winner at equal priority + } + if err := s.putElems(ctx, elems, "block-multi", d); err != nil { + t.Fatal(err) + } + if len(events) != 1 { + t.Fatalf("putHook call count = %d, want 1", len(events)) + } + if !bytes.Equal(events[0].OldValue, []byte("seed")) { + t.Errorf("OldValue = %q, want seed (pre-delta committed value)", events[0].OldValue) + } + if !bytes.Equal(events[0].NewValue, []byte("z")) { + t.Errorf("NewValue = %q, want z (in-delta lex-largest)", events[0].NewValue) + } + if events[0].OldPriority != 1 { + t.Errorf("OldPriority = %d, want 1 (seed's priority)", events[0].OldPriority) + } + if events[0].NewPriority != 2 { + t.Errorf("NewPriority = %d, want 2 (delta priority)", events[0].NewPriority) + } + }) +} + // TestPurgeKeyBlocksDeltaNil verifies that hooks fired from the purge path // receive a nil Delta, since no originating delta triggered the state change. func TestPurgeKeyBlocksDeltaNil(t *testing.T) { From 393dca7f10f71cba88a4c81d16f786f08bcf4b2c Mon Sep 17 00:00:00 2001 From: guillaumemichel Date: Wed, 29 Apr 2026 09:27:52 +0200 Subject: [PATCH 12/12] fix: dedupe putElems by key, fire put hook once per key --- crdt.go | 13 ++-- set.go | 190 +++++++++++++++++++++++++++++--------------------------- 2 files changed, 106 insertions(+), 97 deletions(-) diff --git a/crdt.go b/crdt.go index 0d3efc1..28a4656 100644 --- a/crdt.go +++ b/crdt.go @@ -131,11 +131,14 @@ type Options struct { // been already broadcasted by other replicas in the // interval. Default: 1m. RebroadcastInterval time.Duration - // PutHook is triggered whenever an element is successfully added to the - // datastore (either by a local or remote update), and only when that - // addition is considered the prevalent value. For partial-tombstone puts - // (where a tombstone removed the previous winner and a surviving element - // took over), the hook fires with the surviving value. Default: nil. + // PutHook is triggered whenever the materialised view for a key is + // updated (either by a local or remote update). It fires once per key + // per merged delta, even when the delta carries multiple elements for + // the same key — in that case the lex-largest value wins at the shared + // delta priority and only the winner is reported. For partial-tombstone + // puts (where a tombstone removed the previous winner and a surviving + // element took over), the hook fires with the surviving value. + // Default: nil. // // PutEvent.OldValue and PutEvent.OldPriority are populated only when // HookLoadPreviousValue is true, otherwise they are zero-valued. When diff --git a/set.go b/set.go index 603cf8b..d9d6331 100644 --- a/set.go +++ b/set.go @@ -372,83 +372,6 @@ func (s *set) setPriority(ctx context.Context, writeStore ds.Write, key string, return writeStore.Put(ctx, prioK, buf[0:n]) } -// sets a value if priority is higher. When equal, it sets if the -// value is lexicographically higher than the current value. -// delta is the triggering delta forwarded to the put hook when the write is -// considered the prevalent value. delta must not be nil; its priority drives -// the write decision. -// setValue writes value at the given priority for key when the delta wins -// (higher priority, or equal priority with a greater byte value). It returns -// a non-nil *PutEvent describing the change when a put hook is registered -// and the write actually happened; otherwise it returns (nil, nil). The -// caller is responsible for firing the hook (typically after the surrounding -// batch is committed, so the hook callback observes post-write state). -func (s *set) setValue(ctx context.Context, writeStore ds.Write, key, id string, value []byte, delta Delta) (*PutEvent, error) { - if delta == nil { - return nil, errors.New("setValue: delta must not be nil") - } - // If this key was tombstoned already, do not store/update the value. - deleted, err := s.inTombsKeyID(ctx, key, id) - if err != nil || deleted { - return nil, err - } - - prio := delta.GetPriority() - curPrio, err := s.getPriority(ctx, key) - if err != nil { - return nil, err - } - - if prio < curPrio { - return nil, nil - } - valueK := s.valueKey(key) - - var oldVal []byte - if prio == curPrio { - oldVal, _ = s.store.Get(ctx, valueK) - // new value greater than old - if bytes.Compare(oldVal, value) >= 0 { - return nil, nil - } - } else if s.hooks.hookLoadPreviousValue && s.hooks.putHook != nil { - // Fetch old value before the write so the hook receives it. - oldVal, _ = s.store.Get(ctx, valueK) - } - - // The oldVal read above always happens when prio == curPrio (needed for - // the Compare). Only forward it to the hook when hookLoadPreviousValue is set. - oldPrio := curPrio - if !s.hooks.hookLoadPreviousValue { - oldVal = nil - oldPrio = 0 - } - - // store value - err = writeStore.Put(ctx, valueK, value) - if err != nil { - return nil, err - } - - // store priority - err = s.setPriority(ctx, writeStore, key, prio) - if err != nil { - return nil, err - } - - if s.hooks.putHook == nil { - return nil, nil - } - return &PutEvent{ - Key: ds.NewKey(key), - OldValue: oldVal, - NewValue: value, - OldPriority: oldPrio, - NewPriority: prio, - Delta: delta, - }, nil -} - // findBestValue looks for all entries for the given key, figures out their // priority from their delta (skipping the blocks by the given pendingTombIDs) // and returns the value with the highest priority that is not tombstoned nor @@ -594,35 +517,118 @@ func (s *set) putElems(ctx context.Context, elems []*pb.Element, id string, delt } } - // Collect put events during the loop and fire the hooks after the batch - // commit so callbacks reading from s.store observe the post-merge - // snapshot. Mirrors putTombs. nil when no put hook is registered, so the - // firing loop becomes a no-op. - var events []PutEvent - if s.hooks.putHook != nil { - events = make([]PutEvent, 0, len(elems)) + // Per-key accumulator: resolve the winning value across all same-key + // elements in this delta before writing, so the store and the put hook + // reflect CRDT semantics (lex-largest wins at tied priority) independent + // of element order, and so the hook fires once per key rather than once + // per element. + type putKeyState struct { + bestVal []byte + skipWrite bool // delta loses to the current store winner for this key + prev keyState + prevLoaded bool // prev snapshot has been read from the store } + prio := delta.GetPriority() + loadPrev := s.hooks.hookLoadPreviousValue && s.hooks.putHook != nil + states := make(map[string]*putKeyState) + for _, e := range elems { e.Id = id // overwrite the identifier as it would come unset key := e.GetKey() // /namespace/elems// k := s.elemsPrefix(key).ChildString(id) - err := store.Put(ctx, k, nil) - if err != nil { + if err := store.Put(ctx, k, nil); err != nil { return err } - // update the value if applicable: - // * higher priority than we currently have. - // * not tombstoned before. - evt, err := s.setValue(ctx, store, key, id, e.GetValue(), delta) + // Skip tombstoned elements: they cannot contribute to the in-delta + // winner (mirrors setValue's inTombsKeyID check). + deleted, err := s.inTombsKeyID(ctx, key, id) if err != nil { return err } - if evt != nil { - events = append(events, *evt) + if deleted { + continue + } + + st, ok := states[key] + if !ok { + st = &putKeyState{} + states[key] = st + + curPrio, err := s.getPriority(ctx, key) + if err != nil { + return err + } + if prio < curPrio { + st.skipWrite = true + } else if prio == curPrio { + // Tied priority: the store's current value is a competitor in + // the lex-largest tiebreak. Seed bestVal with it and snapshot + // it as prev so the post-loop no-op check can suppress a + // redundant write when no in-delta elem beats curVal. + curVal, _ := s.store.Get(ctx, s.valueKey(key)) + st.bestVal = curVal + st.prev = keyState{value: curVal, priority: curPrio} + st.prevLoaded = true + } else { + // prio > curPrio: delta always wins, no tiebreak needed + // bestVal stays nil and will be replaced by any in-delta value. + if loadPrev { + curVal, _ := s.store.Get(ctx, s.valueKey(key)) + st.prev = keyState{value: curVal, priority: curPrio} + st.prevLoaded = true + } + } + } + + if st.skipWrite { + continue + } + + if v := e.GetValue(); bytes.Compare(v, st.bestVal) > 0 { + st.bestVal = v + } + } + + var events []PutEvent + if s.hooks.putHook != nil { + events = make([]PutEvent, 0, len(states)) + } + + for key, st := range states { + if st.skipWrite { + continue + } + // Suppress no-op writes: when the chosen value and priority match + // what is already in the store (tied priority, equal bytes), nothing + // changes. Mirrors setValue's equal-bytes skip. + if st.prevLoaded && prio == st.prev.priority && bytes.Equal(st.bestVal, st.prev.value) { + continue + } + + if err := store.Put(ctx, s.valueKey(key), st.bestVal); err != nil { + return err + } + if err := s.setPriority(ctx, store, key, prio); err != nil { + return err + } + + if s.hooks.putHook == nil { + continue + } + evt := PutEvent{ + Key: ds.NewKey(key), + NewValue: st.bestVal, + NewPriority: prio, + Delta: delta, + } + if loadPrev { + evt.OldValue = st.prev.value + evt.OldPriority = st.prev.priority } + events = append(events, evt) } if batching {