Merge pull request #491 from bootjp/feat/jepsen-schedule-enhance

fix: remove HLC from snapshotTS to prevent lost-update on concurrent writes

Author: bootjp

adapter/ts.go

@@ -5,20 +5,18 @@ import (
 	"github.com/bootjp/elastickv/store"
 )
 
-// snapshotTS picks a safe snapshot timestamp:
-// - uses the store's last commit watermark if available,
-// - otherwise the coordinator's HLC current value,
-// - and falls back to MaxUint64 if neither is set.
-func snapshotTS(clock *kv.HLC, st store.MVCCStore) uint64 {
+// snapshotTS picks a safe snapshot timestamp based solely on the store's
+// last committed watermark. The HLC must NOT be used here because
+// clock.Current() can advance ahead of the committed state (e.g. when a
+// concurrent write obtains its commitTS via clock.Next()). Using the HLC
+// would give a startTS that does not reflect what was actually read,
+// allowing the write-conflict check (latestTS > startTS) to miss
+// conflicts when startTS == a concurrent transaction's commitTS.
+func snapshotTS(_ *kv.HLC, st store.MVCCStore) uint64 {
 	ts := uint64(0)
 	if st != nil {
 		ts = st.LastCommitTS()
 	}
-	if clock != nil {
-		if cur := clock.Current(); cur > ts {
-			ts = cur
-		}
-	}
 	if ts == 0 {
 		ts = ^uint64(0)
 	}

adapter/ts_test.go

@@ -0,0 +1,50 @@
+package adapter
+
+import (
+	"context"
+	"testing"
+
+	"github.com/bootjp/elastickv/kv"
+	"github.com/bootjp/elastickv/store"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestSnapshotTSDoesNotUseHLC(t *testing.T) {
+	st := store.NewMVCCStore()
+	require.NoError(t, st.PutAt(context.Background(), []byte("key"), []byte("v"), 10, 0))
+
+	clock := kv.NewHLC()
+	clock.Observe(999) // Advance HLC far beyond LastCommitTS
+
+	ts := snapshotTS(clock, st)
+	assert.Equal(t, st.LastCommitTS(), ts,
+		"snapshotTS must return LastCommitTS, not HLC.Current()")
+	assert.Less(t, ts, uint64(999),
+		"snapshotTS must not advance to HLC value")
+}
+
+func TestSnapshotTSFallbackWhenStoreEmpty(t *testing.T) {
+	clock := kv.NewHLC()
+	st := store.NewMVCCStore()
+
+	ts := snapshotTS(clock, st)
+	assert.Equal(t, ^uint64(0), ts,
+		"snapshotTS must return MaxUint64 when store is empty")
+}
+
+func TestSnapshotTSNilStore(t *testing.T) {
+	clock := kv.NewHLC()
+	ts := snapshotTS(clock, nil)
+	assert.Equal(t, ^uint64(0), ts,
+		"snapshotTS must return MaxUint64 when store is nil")
+}
+
+func TestSnapshotTSNilClock(t *testing.T) {
+	st := store.NewMVCCStore()
+	require.NoError(t, st.PutAt(context.Background(), []byte("k"), []byte("v"), 42, 0))
+
+	ts := snapshotTS(nil, st)
+	assert.Equal(t, uint64(42), ts,
+		"snapshotTS must work with nil clock")
+}