feat: add onPut/onDelete hooks with correct tombstone hook semantics

[guillaumemichel]

Summary

This PR reworks the CRDT set hook system: PutHook and DeleteHook are given richer event-struct signatures, a new HookLoadPreviousValue option exposes the pre-write state, and several correctness issues in how hooks are triggered during tombstone processing are fixed.

Breaking change

The PutHook and DeleteHook signatures are changed. This is an intentional breaking change: extending the existing hooks is cleaner than introducing a parallel set.

Before:

PutHook    func(key ds.Key, value []byte)
DeleteHook func(key ds.Key)

After:

PutHook    func(PutEvent)
DeleteHook func(DeleteEvent)

Where PutEvent carries Key, NewValue, NewPriority, optionally OldValue/OldPriority (when HookLoadPreviousValue is set), and the triggering Delta. DeleteEvent carries Key, optionally LastValue/LastPriority, and the triggering Delta.

Callers upgrading need to update their hook signatures accordingly. See examples/globaldb/globaldb.go for an updated example.

New HookLoadPreviousValue option

A new boolean option, HookLoadPreviousValue (default false), controls whether the previous value and priority for the key are read from the store before a hook-relevant write, so they can be forwarded to the hook:

• When false: OldValue/OldPriority (put) and LastValue/LastPriority (delete) are left zero-valued. No extra read is performed.
• When true: one extra datastore read is done per triggered hook to populate those fields, and the suppression rules below kick in.

PutHook may fire during tombstone processing (new)

A tombstone does not always mean a delete. When multiple elements share a key and only some are tombstoned, the surviving element with the highest priority becomes the new value. In that case PutHook fires (not DeleteHook) because the key remains in the set with a (potentially different) value. Callers should be aware that receiving a put hook during a tombstone delta is valid and expected.

To tell the two cases apart, compare PutEvent.NewPriority with PutEvent.Delta.GetPriority(): equal means the value was written by this delta; lower means an older element resurfaced after a partial tombstone.

Hook suppression rules (only when HookLoadPreviousValue is true)

Two suppression rules prevent spurious hook calls:

1. No put hook when the winning element is unchanged: if a partial tombstone leaves the same surviving value and priority as before, PutHook does not fire. A same-value / different-priority swap still fires the hook.
2. No delete hook when the key had no prior value: if a tombstone targets a key that was never in the set (or was already fully deleted), DeleteHook does not fire. Hooks only signal actual state transitions. Note that the delete hook is triggered when a legitimately-stored empty value is removed.

These rules are only active when HookLoadPreviousValue is set, since that's when the pre-state is known.

New tests

Added tests covering various putTombs() scenarios using the hooks in set_test.go.

[gammazero]

The delete hook (deleteHook/onDelete) could be removed entirely...

I think it helps simplify things to have only one representation of things, and in that regard this seems like a good idea, so as long as there is no other purpose for a nil value. Was there ever an intended purpose of a nil value, since they are allowed?

[guillaumemichel]

The delete hook (deleteHook/onDelete) could be removed entirely...

I think it helps simplify things to have only one representation of things, and in that regard this seems like a good idea, so as long as there is no other purpose for a nil value. Was there ever an intended purpose of a nil value, since they are allowed?

Good point, empty values are actually allowed IIUC so we have to keep the delete hooks.

[hsanjuan]

1. No put hook when value is unchanged: if a partial tombstone results in the same surviving value as before, neither putHook nor onPut fires.

I don't know where I stand on this. There might be some cases when it is important to trigger the hook on "overwrites", even with the same value, as a way to signal that the key has been written again.

Known limitation: nil values are indistinguishable from "no value" in findBestValue()

This sounds like a bug worth fixing at some point.

[hsanjuan]

So, while functionality wise seems ok, plainly speaking I think it is ugly to have putHook and onPut, with similar semantics, but different, and mutually exclusive. I don't have a good answer though.

[hsanjuan]

1. No put hook when value is unchanged: if a partial tombstone results in the same surviving value as before, neither putHook nor onPut fires.

I don't know where I stand on this. There might be some cases when it is important to trigger the hook on "overwrites", even with the same value, as a way to signal that the key has been written again.

Sorry, I read now that it only affects tombstones, normal puts retain behavior.

--

So in principle, it seems to me that the old hooks could adopt the semantic/behavior of the new hooks, with the main difference being the inclusion of the old value, which requires an extra read from the db? I wonder if we can consolidate into a single put/delete hook definition, and make the "old value reading" an option.

[guillaumemichel]

Thanks for the review @hsanjuan! I consolidated the new hooks interface to the existing hooks. It breaks the interface but it is cleaner than adding new hooks.

Let me know your thoughts.

[guillaumemichel]

I updated the PR description to reflect the latest state.

Concerning consolidating put and delete hooks into a single hook, it is possible to use the current PutHook only and set PutEvent.NewPriority to 0 to signal the value was removed. A single hook would simplify go-ds-crdt but would push the complexity of identifying Put/Delete to the hook caller, so I lean toward keeping 2 distinct hooks. Happy to hear other opinions.

[sergey-cheperis]

@guillaumemichel Thank you for this set of changes. Going to be really useful. As far as I can see, there seems to be an inconsistency between putElems and putTombs logic where putTombs triggers the hooks after the batch commit but putElems does that before. This way, if the hook reads from the store, it can't tell whether it's reading from the post-merge or pre-merge snapshot. Is it an intentional design decision?

[guillaumemichel]

Good point @sergey-cheperis! Updated so that putElems call the put hook is called after batch commit. Now all hooks are called post-merge. See f45c196

[sergey-cheperis]

@guillaumemichel Looks great, thank you!

[gammazero]

I think this is correct. Double check the you want to set st.bestVal when v > st.bestVal