diff --git a/Makefile b/Makefile index 02e46e5..7cd0e9e 100644 --- a/Makefile +++ b/Makefile @@ -15,6 +15,15 @@ ragel: lint: golangci-lint run ./... +# Model-check the replication protocol spec (needs tla2tools.jar, see tla/README.md) +TLA2TOOLS ?= tla2tools.jar +.PHONY: tla +tla: + cd tla && for cfg in MCFixed MCFixedChurn MCFixedLast MCWitness MCPing MCBatchFixed MCBatchFixedLast; do \ + echo "=== $$cfg ==="; \ + java -XX:+UseParallelGC -cp $(TLA2TOOLS) tlc2.TLC -config $$cfg.cfg -workers auto -deadlock MCChotkiSync || true; \ + done + .PHONY: update-pebble update-pebble: go mod edit -replace github.com/cockroachdb/pebble=github.com/drpcorg/pebble@master diff --git a/README.md b/README.md index e2d324a..a8e3174 100644 --- a/README.md +++ b/README.md @@ -79,6 +79,10 @@ Internally, Chotki is [pebble db][p] using [RDX](./rdx) merge operators. See the RDX doc for further details on its serialization format (type-length-value) and a very predictable choice of CRDTs. +The replication protocol is described in [replication](./replication) +and has a formal TLA+ specification in [tla](./tla), model-checked with +TLC (including reproductions of historical protocol bugs). + ## Comparison to other projects Overall, RDX is the final version of RON (Replicated Object diff --git a/chotki.go b/chotki.go index 05eb53f..58c58ab 100644 --- a/chotki.go +++ b/chotki.go @@ -113,6 +113,7 @@ type Options struct { WriteTimeout time.Duration TlsConfig *tls.Config MaxSyncDuration time.Duration + CounterSyncPeriod time.Duration // flush+reload cadence for the counter manager } func (o *Options) SetDefaults() { @@ -153,6 +154,11 @@ func (o *Options) SetDefaults() { o.MaxSyncDuration = 10 * time.Minute } + // Run treats <= 0 as "no ticker", so normalize negatives too. + if o.CounterSyncPeriod <= 0 { + o.CounterSyncPeriod = time.Second + } + o.Merger = &pebble.Merger{ Name: "CRDT", Merge: func(key, value []byte) (pebble.ValueMerger, error) { @@ -193,6 +199,12 @@ type syncPoint struct { // so any later reader can detect this and skip the apply. batch *pebble.Batch start time.Time + // via is the id of the replication session whose 'H' record created + // this sync point; the sync point is aborted when that session ends + // (AbortSyncsVia), as its remaining 'D'/'V' records die with the + // session's connection. + via string + created []rdx.ID // objects created in this sync; reindexed from merged state after 'V' } // closeLocked closes the batch if it's still open. Must be called with s.mu held. @@ -212,20 +224,34 @@ func (s *syncPoint) close() { // Main Chotki struct type Chotki struct { + // last is the cached local id allocator state: CommitPacket stamps + // the next local mutation from it. It is advanced both by local + // commits (under commitMutex) and by replication sessions draining + // records stamped with our own src (e.g. our own history synced back + // after a restore from an older snapshot) — those paths do not share + // a lock, so last has its own one (lastLock). An unsynchronized read + // here can reuse an already issued seq: two different mutations under + // one rdx.ID (see tla/README.md, bug 4). + lastLock sync.Mutex last rdx.ID src uint64 clock rdx.Clock cancelCtx context.CancelFunc waitGroup *sync.WaitGroup - lock sync.RWMutex - commitMutex sync.Mutex + lock sync.RWMutex + commitMutex sync.Mutex + // seqWriteMu backs Start/EndSequentialWrite (host.Host): the cooperative + // bracket read-modify-write writers hold across their read AND commit. + // Deliberately NOT commitMutex — commit methods lock that internally, so + // the bracket must nest around them (ordering: seqWriteMu → commitMutex). + seqWriteMu sync.Mutex db *pebble.DB net *network.Net dir string opts Options log utils.Logger - counterCache sync.Map + counters *counters.AtomicCounterManager IndexManager *indexes.IndexManager outq *xsync.MapOf[string, protocol.DrainCloser] // queues to broadcast all new packets @@ -277,6 +303,10 @@ func (cho *Chotki) cleanSyncs(ctx context.Context) { } } +// reindexPeriod is how often the background reindex worker scans for tasks. +// A var so tests can set it long, isolating the apply path from the backstop. +var reindexPeriod = time.Second + // Opens a new Chotki instance. func Open(dirname string, opts Options) (*Chotki, error) { exists, err := Exists(dirname) @@ -315,6 +345,15 @@ func Open(dirname string, opts Options) (*Chotki, error) { waitGroup: &wg, } + // If Open fails after workers start, the caller has no *Chotki to Close; + // tear down via Close (nil-safe on a partial instance, single teardown path). + opened := false + defer func() { + if !opened { + _ = cho.Close() + } + }() + cho.net = network.NewNet(cho.log, func(name string) protocol.FeedDrainCloserTraced { // new connection @@ -358,9 +397,10 @@ func Open(dirname string, opts Options) (*Chotki, error) { cho.IndexManager = indexes.NewIndexManager(&cho) wg.Add(1) // reindex tasks are checked in a separate worker + reindexP := reindexPeriod go func() { defer wg.Done() - cho.IndexManager.CheckReindexTasks(ctx) + cho.IndexManager.CheckReindexTasks(ctx, reindexP) }() wg.Add(1) @@ -370,6 +410,14 @@ func Open(dirname string, opts Options) (*Chotki, error) { cho.cleanSyncs(ctx) }() + cho.counters = counters.NewAtomicCounterManager(&cho, opts.CounterSyncPeriod, cho.log) + wg.Add(1) + // counters are periodically flushed and reloaded in a separate worker + go func() { + defer wg.Done() + cho.counters.Run(ctx) + }() + if !exists { id0 := rdx.IDFromSrcSeqOff(opts.Src, 0, 0) // apply log0, some default objects for all replicas @@ -385,7 +433,7 @@ func Open(dirname string, opts Options) (*Chotki, error) { protocol.Record('S', rdx.Stlv("")), )) - if err = cho.drain(context.Background(), init); err != nil { + if _, err = cho.drain(context.Background(), init, nil); err != nil { return nil, errors.Join(err, fmt.Errorf("unable to drain initial data to chotki")) } } @@ -397,6 +445,7 @@ func Open(dirname string, opts Options) (*Chotki, error) { cho.last = vv.GetID(cho.src) + opened = true return &cho, nil } @@ -418,6 +467,10 @@ func (cho *Chotki) Close() error { cho.net = nil } + // commitMutex lets an in-flight local commit (which holds it, not cho.lock) + // finish its drain + reindex reads on cho.db before we close and nil it. + // Network drains are guarded by cho.lock; the shutdown flush already ran. + cho.commitMutex.Lock() if cho.db != nil { if err := cho.db.Close(); err != nil { cho.log.Error("couldn't close Pebble", "err", err) @@ -425,6 +478,7 @@ func (cho *Chotki) Close() error { cho.db = nil } + cho.commitMutex.Unlock() cho.outq.Clear() cho.syncs.Range(func(id rdx.ID, s *syncPoint) bool { @@ -436,15 +490,29 @@ func (cho *Chotki) Close() error { cho.types.Clear() cho.src = 0 + cho.lastLock.Lock() cho.last = rdx.ID0 + cho.lastLock.Unlock() return nil } -// Returns an atomic counter object that can be used to increment/decrement a counter. -func (cho *Chotki) Counter(rid rdx.ID, offset uint64, updatePeriod time.Duration) *counters.AtomicCounter { - counter, _ := cho.counterCache.LoadOrStore(rid.ToOff(offset), counters.NewAtomicCounter(cho, rid, offset, updatePeriod)) - return counter.(*counters.AtomicCounter) +// Counter returns a lock-free handle for the field; increments are flushed periodically and on Close (hard crash loses them). +func (cho *Chotki) Counter(rid rdx.ID, offset uint64) *counters.AtomicCounter { + return cho.counters.Counter(rid, offset) +} + +// SyncCounters forces an immediate flush+reload cycle of all counters. +func (cho *Chotki) SyncCounters(ctx context.Context) { + cho.counters.Cycle(ctx) +} + +// ObjectIDByHash resolves an object's id from a hash-indexed first-field value (e.g. a +// UUID) without allocating a snapshot: it serves from the IndexManager's hash cache +// (invalidated as objects change) and reads the live DB only on a miss. For hot paths +// that need just the id, not a consistent object read. +func (cho *Chotki) ObjectIDByHash(cid rdx.ID, fid uint32, fieldValue []byte) (rdx.ID, error) { + return cho.IndexManager.GetByHash(cid, fid, fieldValue, cho.db) } // Returns the source id of the Chotki instance. @@ -459,9 +527,31 @@ func (cho *Chotki) Clock() rdx.Clock { // Returns the latest used rdx.ID of current replica func (cho *Chotki) Last() rdx.ID { + cho.lastLock.Lock() + defer cho.lastLock.Unlock() return cho.last } +// nextLast atomically allocates the next local id, advancing the cached +// allocator state. +func (cho *Chotki) nextLast() rdx.ID { + cho.lastLock.Lock() + defer cho.lastLock.Unlock() + cho.last = cho.last.IncPro(1).ZeroOff() + return cho.last +} + +// advanceLast bumps the cached allocator to id if it is behind. Called by drain +// only after the own-source ids up to id have been durably applied, so cho.last +// never runs ahead of the persisted version vector. +func (cho *Chotki) advanceLast(id rdx.ID) { + cho.lastLock.Lock() + defer cho.lastLock.Unlock() + if cho.last.Less(id) { + cho.last = id + } +} + // Returns the write options of the Chotki instance. func (cho *Chotki) WriteOptions() *pebble.WriteOptions { return cho.opts.PebbleWriteOptions @@ -600,19 +690,76 @@ func (cho *Chotki) CommitPacket(ctx context.Context, lit byte, ref rdx.ID, body if cho.db == nil { return rdx.BadId, chotki_errors.ErrClosed } - // create new last id - id = cho.last.IncPro(1).ZeroOff() + // allocate the next local id + id = cho.nextLast() i := protocol.Record('I', id.ZipBytes()) // this is typically some higher order structure (like for object it will be class id, for field update it will be object id) r := protocol.Record('R', ref.ZipBytes()) packet := protocol.Record(lit, i, r, protocol.Join(body...)) recs := protocol.Records{packet} - err = cho.drain(ctx, recs) + // single packet = one object; no intra-batch cross-object uniqueness to check + _, err = cho.drain(ctx, recs, nil) DrainTime.WithLabelValues("commit").Observe(float64(time.Since(now)) / float64(time.Millisecond)) + if err != nil { + // Don't publish an id we failed to persist: a crash before the next + // successful commit could reissue it while a peer already holds it. + return + } cho.Broadcast(ctx, recs, "") return } +// CommitBatch applies edits as one all-or-nothing Pebble batch + broadcast. Each edit is stamped in +// slice order; correctness relies on 'E' ops being commutative Merge calls (no read-back). +func (cho *Chotki) CommitBatch(ctx context.Context, edits []host.Edit) (err error) { + if len(edits) == 0 { + return nil + } + // prevent cancellation as it can make this function non atomic + ctx = context.WithoutCancel(ctx) + + now := time.Now() + defer func() { + DrainTime.WithLabelValues("commitbatch").Observe(float64(time.Since(now)) / float64(time.Millisecond)) + }() + cho.commitMutex.Lock() + defer cho.commitMutex.Unlock() + + if cho.db == nil { + return chotki_errors.ErrClosed + } + + recs := make(protocol.Records, 0, len(edits)) + for _, e := range edits { + id := cho.nextLast() // one id per edit + recs = append(recs, protocol.Record('E', + protocol.Record('I', id.ZipBytes()), + protocol.Record('R', e.Ref.ZipBytes()), + protocol.Join(e.Body...))) + } + // drain applies the batch but doesn't broadcast; we do that below. + // claims rejects two edits setting the same unique value on different + // objects before the batch commits. + claims := map[string]rdx.ID{} + if _, err = cho.drain(ctx, recs, claims); err != nil { + return err + } + cho.Broadcast(ctx, recs, "") + return nil +} + +// StartSequentialWrite acquires the cooperative read-modify-write bracket (see +// host.Host): hold it across both the read and commit of a state-dependent op +// (e.g. a Z-counter set). Commit methods don't take it. Not reentrant. +func (cho *Chotki) StartSequentialWrite() { + cho.seqWriteMu.Lock() +} + +// EndSequentialWrite releases the bracket taken by StartSequentialWrite. +func (cho *Chotki) EndSequentialWrite() { + cho.seqWriteMu.Unlock() +} + type NetCollector struct { net *network.Net read_buffers_size *prometheus.Desc @@ -708,70 +855,114 @@ func (cho *Chotki) Metrics() []prometheus.Collector { } } -// Handles all updates and actually writes the to storage. -// the allowed types are 'C', 'O', 'E', 'H', 'D', 'V', 'B', 'P', 'Y' -// do not confuse it with RDX types -func (cho *Chotki) drain(ctx context.Context, recs protocol.Records) (err error) { +// Handles all updates and writes them to storage; types 'C','O','E','H','D', +// 'V','B','P','Y' (not RDX types). Drop-on-error: the Y/C/O/E records apply as +// one all-or-nothing batch, so applied is len(recs) on success, 0 on error, and +// replication relays only a fully-persisted batch. claims (CommitBatch only) +// rejects intra-batch duplicate unique values. +func (cho *Chotki) drain(ctx context.Context, recs protocol.Records, claims map[string]rdx.ID) (applied int, err error) { EventsMetric.Add(float64(len(recs))) var calls []CallHook - for _, packet := range recs { // parse the packets + // One batch for the non-sync packets (Y/C/O/E), applied once after the loop. + // Sync packets (H/D/V) use their own per-sync batch, committed on 'V'. + pb := pebble.Batch{} + classChanged := false + var created []rdx.ID // objects created here; reindexed post-commit + var syncCreated []rdx.ID // objects created by diff-sync 'V's; reindexed post-flush + // Highest own-source id; cho.last advances to it only after pb is durable + // (in flush), never in the loop, or a crash could reissue an id. + var maxOwn rdx.ID + // flush persists pb, then advances the allocator. Deferred so it runs on + // every exit path. On error pb (an in-memory buffer) is dropped and applied + // zeroed: nothing relayed, allocator unchanged. + flushed := false + flush := func() { + if flushed { + return + } + flushed = true + if err != nil { + applied = 0 + return + } + if pb.Count() > 0 { + if ae := cho.db.Apply(&pb, cho.opts.PebbleWriteOptions); ae != nil { + err = ae + applied = 0 + return + } + } + if maxOwn != rdx.ID0 { + cho.advanceLast(maxOwn) + } + } + defer flush() + for _, packet := range recs { if err != nil { break } - // parse the packet to understand what kind of packet is it lit, id, ref, body, parseErr := replication.ParsePacket(packet) if parseErr != nil { cho.log.WarnCtx(ctx, "bad packet", "err", parseErr) - return parseErr + return applied, parseErr } - // if this is a packet commited by our replica we need to update our last id - // as current commit holds the mutex, it is safe to update this id - if id.Src() == cho.src && cho.last.Less(id) { + // An own-src record must advance the allocator or future commits reuse + // its seq. Sources: local commits, or a session draining our own history + // back (e.g. after a restore). cho.last advances after the durable apply. + if id.Src() == cho.src && maxOwn.Less(id) { if id.Off() != 0 { - return rdx.ErrBadPacket + return applied, rdx.ErrBadPacket } - cho.last = id + maxOwn = id } - // noApply can be set to true if we don't want to apply the batch - pb, noApply := pebble.Batch{}, false - cho.log.DebugCtx(ctx, "new packet", "type", string(lit), "packet", id.String()) switch lit { case 'Y': // creates a replica log if ref != rdx.ID0 { - return ErrBadYPacket + return applied, ErrBadYPacket } - err = cho.ApplyOY('Y', id, ref, body, &pb) + err = cho.ApplyOY('Y', id, ref, body, &pb, claims) case 'C': // creates a class err = cho.ApplyC(id, ref, body, &pb, &calls) if err == nil { - // clear cache for classes if class changed - cho.types.Clear() + // clear the type cache after the batch applies, so a concurrent + // rebuild can't repopulate it from the pre-class state. + classChanged = true } case 'O': // creates an object if ref == rdx.ID0 { - return ErrBadOPacket + return applied, ErrBadOPacket + } + err = cho.ApplyOY('O', id, ref, body, &pb, claims) + if err == nil { + created = append(created, id) } - err = cho.ApplyOY('O', id, ref, body, &pb) case 'E': // edits an object if ref == rdx.ID0 { - return ErrBadEPacket + return applied, ErrBadEPacket } - err = cho.ApplyE(id, ref, body, &pb, &calls) + // created also collects edit targets whose class was unresolvable + // mid-batch (object's 'O' not visible yet); reindexed post-commit. + err = cho.ApplyE(id, ref, body, &pb, &calls, &created, claims) case 'H': // handshake d := cho.db.NewBatch() - // we also create a new sync point, pebble batch that we will write diffs to + // One diff sync per src: a new handshake displaces the origin's + // stale sync points (keyed by snapshot id, so match on id.Src()). + incomingVia := replication.SessionIdFromCtx(ctx) activeSyncs := make([]rdx.ID, 0) cho.syncs.Range(func(key rdx.ID, value *syncPoint) bool { - if key.Src() == cho.src { + // Displace only on a genuine supersede: same session (restart) or + // strictly older snapshot. Evicting a live diff via another session + // would flap a cyclic topology; a tree is assumed (see README). + if key.Src() == id.Src() && (value.via == incomingVia || key.Less(id)) { activeSyncs = append(activeSyncs, key) } return true @@ -784,16 +975,24 @@ func (cho *Chotki) drain(ctx context.Context, recs protocol.Records) (err error) cho.log.InfoCtx(ctx, "deleted active sync", "id", s.String()) } err = cho.ApplyH(id, ref, body, d) - // Store after ApplyH so no goroutine can Load the sync point - // until the batch is fully initialized. - cho.syncs.Store(id, &syncPoint{batch: d, start: time.Now()}) - cho.log.InfoCtx(ctx, "created new sync point", "id", id.String()) + if err == nil { + // Store after ApplyH so no goroutine can Load the sync point + // until the batch is fully initialized. + cho.syncs.Store(id, &syncPoint{ + batch: d, + start: time.Now(), + via: replication.SessionIdFromCtx(ctx), + }) + cho.log.InfoCtx(ctx, "created new sync point", "id", id.String()) + } else { + _ = d.Close() + } case 'D': // diff packet // load sync point if exists s, ok := cho.syncs.Load(id) if !ok { - return ErrSyncUnknown + return applied, ErrSyncUnknown } s.mu.Lock() if s.batch == nil { @@ -803,42 +1002,56 @@ func (cho *Chotki) drain(ctx context.Context, recs protocol.Records) (err error) // partial data loss invisible to the peer, so surface the same // error as for a missing sync point and let the session restart. s.mu.Unlock() - return ErrSyncUnknown + return applied, ErrSyncUnknown } - err = cho.ApplyD(id, ref, body, s.batch) + err = cho.ApplyD(id, ref, body, s.batch, &s.created, claims) s.mu.Unlock() - // we use separate batch, so noApply is true - noApply = true + // applied into the sync point's own batch, not pb case 'V': // version vector sent in the end of diff sync // load sync point if exists s, ok := cho.syncs.Load(id) if !ok { - return ErrSyncUnknown + return applied, ErrSyncUnknown } s.mu.Lock() if s.batch == nil { // See the 'D' case for rationale: batch was closed out from // under us, so treat it the same as an unknown sync. s.mu.Unlock() - return ErrSyncUnknown + return applied, ErrSyncUnknown } DiffSyncSize.Observe(float64(s.batch.Len())) - // update blocks version vectors err = cho.ApplyV(id, ref, body, s.batch) if err == nil { - // apply batch and delete sync point as diff sync is finished + // durability point for everything the diff sync delivered err = cho.db.Apply(s.batch, cho.opts.PebbleWriteOptions) - // db.Apply doesn't return the batch to the pool on its own, - // so close it explicitly to avoid leaking a pool slot. + // db.Apply doesn't return the batch to the pool; close it. _ = s.batch.Close() s.batch = nil + // finished either way; a failed Apply restarts the diff sync. cho.syncs.Delete(id) + } + if err == nil { + // gated on the Apply persisting — a rejected batch wrote nothing. + // + // A diff-sync can carry our own history back (e.g. after a + // restore), advancing the persisted VV but not the allocator; + // advance it from the merged VV so no id is reissued. + if vv, verr := cho.VersionVector(); verr == nil { + cho.advanceLast(vv.GetID(cho.src)) + } else { + cho.log.WarnCtx(ctx, "could not read VV to advance allocator after sync", "err", verr) + } + // A diff-sync may carry now-durable class changes; drop the type + // cache unconditionally (infrequent, cheap to rebuild). + cho.types.Clear() + // A class in pb isn't durable until flush(), so reindex these + // post-flush alongside `created`. + syncCreated = append(syncCreated, s.created...) cho.log.InfoCtx(ctx, "applied diff batch and deleted it", "id", id) } s.mu.Unlock() - // we don't want to apply the batch as we already applied it - noApply = true case 'B': // session end cho.log.InfoCtx(ctx, "received session end", "id", id.String(), "data", string(body)) @@ -849,19 +1062,41 @@ func (cho *Chotki) drain(ctx context.Context, recs protocol.Records) (err error) cho.syncs.Delete(id) case 'P': // ping noop default: - return fmt.Errorf("unsupported packet type %c", lit) + return applied, fmt.Errorf("unsupported packet type %c", lit) } - if !noApply && err == nil { - if err := cho.db.Apply(&pb, cho.opts.PebbleWriteOptions); err != nil { - return err - } + if err == nil { + applied++ } } + // Persist pb and advance the allocator before running hooks that read the + // merged state. flush is idempotent; the deferred call above is now a no-op. + flush() if err != nil { return } + if classChanged { + cho.types.Clear() + } + + // Reindex created objects from their committed values, so an edit that + // arrived before its create is picked up now. Dedup by id: ApplyD/ApplyE + // append one entry per object and per deferred field. + if len(created)+len(syncCreated) > 0 { // the common no-creates drain allocates nothing + syncCreated = append(syncCreated, created...) + seen := make(map[rdx.ID]struct{}, len(syncCreated)) + for _, oid := range syncCreated { + key := oid.ZeroOff() + if _, ok := seen[key]; ok { + continue + } + seen[key] = struct{}{} + if e := cho.IndexManager.IndexObject(key); e != nil { + cho.log.WarnCtx(ctx, "post-commit index failed", "oid", key.String(), "err", e) + } + } + } if len(calls) > 0 { for _, call := range calls { @@ -874,6 +1109,31 @@ func (cho *Chotki) drain(ctx context.Context, recs protocol.Records) (err error) // Public for drain method with some additional metrics func (cho *Chotki) Drain(ctx context.Context, recs protocol.Records) (err error) { + _, err = cho.DrainApplied(ctx, recs) + return +} + +// AbortSyncsVia closes pending diff-sync points created by the given session. +// Its 'D'/'V' packets travel through that connection, so once it's gone the +// batch can't complete; a 'V' relayed later would apply the handshake VV +// without the data that died with it — silent data loss (see tla/). +func (cho *Chotki) AbortSyncsVia(ctx context.Context, sessionId string) { + if sessionId == "" { + return + } + cho.syncs.Range(func(key rdx.ID, s *syncPoint) bool { + if s.via == sessionId { + s.close() + cho.syncs.Delete(key) + cho.log.InfoCtx(ctx, "aborted sync point of a closed session", "id", key.String()) + } + return true + }) +} + +// DrainApplied is Drain plus the applied-record count. Draining is +// all-or-nothing, so the count is len(recs) on success and 0 on error. +func (cho *Chotki) DrainApplied(ctx context.Context, recs protocol.Records) (applied int, err error) { now := time.Now() defer func() { DrainTime.WithLabelValues("drain").Observe(float64(time.Since(now)) / float64(time.Millisecond)) @@ -881,10 +1141,10 @@ func (cho *Chotki) Drain(ctx context.Context, recs protocol.Records) (err error) cho.lock.RLock() defer cho.lock.RUnlock() if cho.db == nil { - return chotki_errors.ErrClosed + return 0, chotki_errors.ErrClosed } EventsBatchSize.Observe(float64(len(recs))) - return cho.drain(ctx, recs) + return cho.drain(ctx, recs, nil) } func dumpKVString(key, value []byte) (str string) { diff --git a/chotki_counter_test.go b/chotki_counter_test.go new file mode 100644 index 0000000..87412a1 --- /dev/null +++ b/chotki_counter_test.go @@ -0,0 +1,48 @@ +package chotki + +import ( + "context" + "testing" + + "github.com/drpcorg/chotki/classes" + "github.com/drpcorg/chotki/protocol" + "github.com/drpcorg/chotki/rdx" + "github.com/stretchr/testify/require" +) + +// An N-counter must not lose increments when a second writer (AddToNField) +// touches the same slot between the manager's read and its flush. +// Without the reload-rebase + read-modify-write flush, the manager writes the +// absolute cached mine, which loses to the max-by-source merge, and never +// adopts the external delta — so Get() under-reports permanently. +func TestNCounterSurvivesConcurrentExternalWriter(t *testing.T) { + dirs, clear := testdirs(0x6e) + defer clear() + a, err := Open(dirs[0], Options{Src: 0x6e, Name: "A"}) + require.NoError(t, err) + defer a.Close() + + cid, err := a.NewClass(context.Background(), rdx.ID0, + classes.Field{Name: "n", RdxType: rdx.Natural}) + require.NoError(t, err) + oid, err := a.NewObjectTLV(context.Background(), cid, protocol.Records{ + protocol.Record(rdx.Natural, rdx.Ntlvt(0, a.Source())), + }) + require.NoError(t, err) + fid := oid.ToOff(1) + + c := a.Counter(oid, 1) + _, err = c.Increment(context.Background(), 1) // manager cache mine=1, unflushed + require.NoError(t, err) + + // External writer raises the same slot before the manager flushes. + _, err = a.AddToNField(context.Background(), fid, 9) + require.NoError(t, err) + + a.SyncCounters(context.Background()) // flush (RMW) + reload + + got, err := c.Get(context.Background()) + require.NoError(t, err) + require.Equal(t, int64(10), got, + "N-counter must reflect both the external +9 and the local +1") +} diff --git a/chotki_drain_atomicity_test.go b/chotki_drain_atomicity_test.go new file mode 100644 index 0000000..500c592 --- /dev/null +++ b/chotki_drain_atomicity_test.go @@ -0,0 +1,126 @@ +package chotki + +import ( + "context" + "testing" + "time" + + "github.com/drpcorg/chotki/host" + "github.com/drpcorg/chotki/protocol" + "github.com/drpcorg/chotki/rdx" + "github.com/drpcorg/chotki/replication" + "github.com/drpcorg/chotki/utils" + "github.com/stretchr/testify/require" +) + +// The batched drain accumulates Y/C/O/E records into one pebble batch applied +// after the loop. On ANY mid-drain error the whole top-level batch is now +// dropped — no partial prefix is ever applied (drop-on-error). So a batch that +// errors partway persists nothing and leaves the allocator exactly where the +// persisted version vector left it: cho.last can never lead the VV, and a +// crash/restart rebuilds cho.last from that same VV. (This reverses the earlier +// flush-on-error behavior, which persisted the applied prefix; see the +// review-fix design doc for the rationale — cluster ①.) +func TestDrainErrorDoesNotOverrunAllocator(t *testing.T) { + dirs, clear := testdirs(0x5c) + defer clear() + + a, err := Open(dirs[0], Options{Src: 0x5c, Name: "replica A"}) + require.NoError(t, err) + defer a.Close() + + // capture a real 'C' packet to reuse its class body in a well-formed + // own-source record below + qa := utils.NewFDQueue[protocol.Records](1<<20, 50*time.Millisecond, 1) + a.outq.Store("capture", qa) + _, err = a.NewClass(context.Background(), rdx.ID0, Schema...) + require.NoError(t, err) + packets := drainQueue(t, qa, 1, 2*time.Second) + require.Len(t, packets, 1) + a.outq.Delete("capture") + _, _, ref, body, err := replication.ParsePacket(packets[0]) + require.NoError(t, err) + + before := a.Last() + + // a well-formed own-source 'C' stamped well ahead of cho.last ... + ownID := rdx.IDFromSrcSeqOff(0x5c, before.Seq()+1000, 0) + require.True(t, before.Less(ownID)) + ownC := protocol.Record('C', + protocol.Record('I', ownID.ZipBytes()), + protocol.Record('R', ref.ZipBytes()), + body) + // ... followed by a 'V' for a sync point that does not exist, which aborts + // the drain with ErrSyncUnknown after the own-source record was staged. + _, vpack := protocol.OpenHeader(nil, 'V') + unknown := rdx.IDFromSrcSeqOff(0x99, 7, 0) + vpack = append(vpack, protocol.Record('T', unknown.ZipBytes())...) + protocol.CloseHeader(vpack, 5) + + err = a.Drain(context.Background(), protocol.Records{ownC, vpack}) + require.ErrorIs(t, err, ErrSyncUnknown) + + vv, err := a.VersionVector() + require.NoError(t, err) + + // New semantics: on ANY mid-drain error the top-level batch is dropped + // wholesale — no partial prefix is ever applied. So the staged own-source + // record is NOT durable, and the allocator did not move. + require.True(t, vv.GetID(0x5c).Less(ownID), + "a dropped batch must not persist the staged own-source record") + require.Equal(t, before, a.Last(), + "the allocator must not advance when the batch is dropped") + + // The core invariant still holds (now trivially): cho.last never sits ahead + // of the persisted version vector. + require.False(t, vv.GetID(0x5c).Less(a.Last()), + "cho.last must not run ahead of the persisted version vector") +} + +// A CommitBatch whose drain errors midway must persist NOTHING (all-or-nothing), +// so a caller that retries the whole batch cannot double-apply a durable prefix +// (the Z-counter read-modify-write doubling). +func TestCommitBatchIsAllOrNothingOnError(t *testing.T) { + dirs, clear := testdirs(0x5d) + defer clear() + a, err := Open(dirs[0], Options{Src: 0x5d, Name: "replica A"}) + require.NoError(t, err) + defer a.Close() + + cid, err := a.NewClass(context.Background(), rdx.ID0, Schema...) + require.NoError(t, err) + oid, err := a.NewObjectTLV(context.Background(), cid, protocol.Records{ + protocol.Record(rdx.String, rdx.Stlv("v0")), + }) + require.NoError(t, err) + + // Persisted VV[src] is the reliable "durable?" signal. We do NOT assert on + // a.Last(): CommitBatch pre-allocates ids via nextLast() BEFORE draining, so + // the in-memory allocator advances even when the batch is dropped (an + // accepted local-commit gap — the ids were never broadcast). + vvBefore, err := a.VersionVector() + require.NoError(t, err) + + // A good edit followed by an edit whose field offset is out of range, which + // makes ApplyE return ErrBadEPacket AFTER the good edit merged into pb. + // ErrBadEPacket is deterministic (packets.go: field > rdx.OffMask). + good := host.Edit{Ref: oid, Body: protocol.Records{ + protocol.Record('F', rdx.ZipUint64(1)), + protocol.Record(rdx.String, rdx.Stlv("v1")), + }} + bad := host.Edit{Ref: oid, Body: protocol.Records{ + protocol.Record('F', rdx.ZipUint64(uint64(rdx.OffMask)+1)), + protocol.Record(rdx.String, rdx.Stlv("v2")), + }} + + err = a.CommitBatch(context.Background(), []host.Edit{good, bad}) + require.Error(t, err) + + // Nothing durable: the batch was dropped wholesale, so the persisted version + // vector did not advance to cover the good edit. (The good edit's ApplyE + // merged a VKey bump into pb, which drop-on-error discards.) + vvAfter, err := a.VersionVector() + require.NoError(t, err) + require.Equal(t, vvBefore.GetID(0x5d), vvAfter.GetID(0x5d), + "a failed CommitBatch must not persist its applied prefix") +} diff --git a/chotki_index_combi_test.go b/chotki_index_combi_test.go new file mode 100644 index 0000000..6f7bfd0 --- /dev/null +++ b/chotki_index_combi_test.go @@ -0,0 +1,283 @@ +package chotki + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/drpcorg/chotki/protocol" + "github.com/drpcorg/chotki/rdx" + testutils "github.com/drpcorg/chotki/test_utils" + "github.com/stretchr/testify/assert" +) + +// capHose captures the records a producer broadcasts, so a single live commit +// (O/E/C) can be replayed into a target replica in a chosen order. +type capHose struct{ recs protocol.Records } + +func (h *capHose) Drain(_ context.Context, recs protocol.Records) error { + h.recs = append(h.recs, recs...) + return nil +} +func (h *capHose) Close() error { return nil } + +// captureBroadcast runs fn (a commit on producer) and returns exactly the +// records it broadcast — the wire packet a peer would receive. +func captureBroadcast(producer *Chotki, fn func()) protocol.Records { + h := &capHose{} + producer.outq.Store("cap", h) + defer producer.outq.Delete("cap") + fn() + return h.recs +} + +// openIdx opens a replica with the reindex worker effectively idle (a very long +// period), so tests assert that the apply path keeps hash indexes correct on its +// own — the reindex backstop can't heal what the apply path is supposed to index. +func openIdx(t *testing.T, dir string, src uint64) *Chotki { + t.Helper() + reindexPeriod = time.Hour + t.Cleanup(func() { reindexPeriod = time.Second }) + c, err := Open(dir, Options{ + Src: src, + Name: "r", + ReadAccumTimeLimit: 100 * time.Millisecond, + }) + assert.NoError(t, err) + return c +} + +// liveUnits sets up a producer and captures the wire packets for: creating a +// hash-indexed class (C), creating an object with value "v1" (O), and editing +// the indexed field to "v2" (E). Returns the class id, object id, and packets. +func liveUnits(t *testing.T, p *Chotki) (cid, rid rdx.ID, cRecs, oRecs, eRecs protocol.Records) { + t.Helper() + ctx := context.Background() + + cRecs = captureBroadcast(p, func() { + var err error + cid, err = p.NewClass(ctx, rdx.ID0, SchemaIndex...) + assert.NoError(t, err) + }) + oRecs = captureBroadcast(p, func() { + var err error + rid, err = p.NewObjectTLV(ctx, cid, protocol.Records{protocol.Record('S', rdx.Stlv("v1"))}) + assert.NoError(t, err) + }) + _, oldTlv, err := p.ObjectFieldTLV(rid.ToOff(1)) + assert.NoError(t, err) + eRecs = captureBroadcast(p, func() { + _, err := p.EditFieldTLV(ctx, rid.ToOff(1), protocol.Record('S', rdx.Sdelta(oldTlv, "v2", p.Clock()))) + assert.NoError(t, err) + }) + return +} + +// assertResolves checks that the hash index on target maps the CURRENT value +// "v2" to the object — the invariant that must hold from the apply path alone. +func assertResolves(t *testing.T, target *Chotki, cid, rid rdx.ID, msg string) { + t.Helper() + got, err := target.ObjectIDByHash(cid, 1, []byte("v2")) + assert.NoError(t, err, msg) + assert.Equal(t, rid, got, msg) +} + +func cat(groups ...protocol.Records) protocol.Records { + out := protocol.Records{} + for _, g := range groups { + out = append(out, g...) + } + return out +} + +// [O,E] arrive in ONE drain. Passes today: ApplyOY's AddFullScanIndex populates +// the in-memory classCache, so E (processed after O) resolves its class. +func TestIndexConverge_LiveSameBatch_OE(t *testing.T) { + dirs, clear := testdirs(0xa, 0xb) + defer clear() + ctx := context.Background() + + p := openIdx(t, dirs[0], 0xa) + defer p.Close() + cid, rid, cRecs, oRecs, eRecs := liveUnits(t, p) + + target := openIdx(t, dirs[1], 0xb) + defer target.Close() + assert.NoError(t, target.Drain(ctx, cRecs)) + assert.NoError(t, target.Drain(ctx, cat(oRecs, eRecs))) + + assertResolves(t, target, cid, rid, "same-batch [O,E] must index current value v2") +} + +// [E,O] in ONE drain: E is processed before O, so the classCache isn't populated +// yet and E's index maintenance is skipped. O then indexes only its own "v1". +func TestIndexConverge_LiveSameBatch_EO(t *testing.T) { + dirs, clear := testdirs(0xa, 0xb) + defer clear() + ctx := context.Background() + + p := openIdx(t, dirs[0], 0xa) + defer p.Close() + cid, rid, cRecs, oRecs, eRecs := liveUnits(t, p) + + target := openIdx(t, dirs[1], 0xb) + defer target.Close() + assert.NoError(t, target.Drain(ctx, cRecs)) + assert.NoError(t, target.Drain(ctx, cat(eRecs, oRecs))) + + assertResolves(t, target, cid, rid, "same-batch [E,O] must index current value v2") +} + +// E arrives before its create O (separate drains, out-of-order delivery). +func TestIndexConverge_LiveEbeforeO(t *testing.T) { + dirs, clear := testdirs(0xa, 0xb) + defer clear() + ctx := context.Background() + + p := openIdx(t, dirs[0], 0xa) + defer p.Close() + cid, rid, cRecs, oRecs, eRecs := liveUnits(t, p) + + target := openIdx(t, dirs[1], 0xb) + defer target.Close() + assert.NoError(t, target.Drain(ctx, cRecs)) + assert.NoError(t, target.Drain(ctx, eRecs)) + assert.NoError(t, target.Drain(ctx, oRecs)) + + assertResolves(t, target, cid, rid, "E-before-O must index current value v2") +} + +// A NEW object arrives via diff sync at a replica that already has the class +// (reindex worker OFF). Does the sync apply path index it, or does it need help? +func TestIndexConverge_DiffSyncNewObject(t *testing.T) { + dirs, clear := testdirs(0xa, 0xb) + defer clear() + ctx := context.Background() + + p := openIdx(t, dirs[0], 0xa) + defer p.Close() + target := openIdx(t, dirs[1], 0xb) + defer target.Close() + + // Target gets the class first (existing-class replica), no objects yet. + cid, err := p.NewClass(ctx, rdx.ID0, SchemaIndex...) + assert.NoError(t, err) + _ = testutils.SyncData(p, target) // returns io.EOF on normal completion + + // A new object is created on p AFTER target already has the class. + rid, err := p.NewObjectTLV(ctx, cid, protocol.Records{protocol.Record('S', rdx.Stlv("v1"))}) + assert.NoError(t, err) + _, oldTlv, err := p.ObjectFieldTLV(rid.ToOff(1)) + assert.NoError(t, err) + _, err = p.EditFieldTLV(ctx, rid.ToOff(1), protocol.Record('S', rdx.Sdelta(oldTlv, "v2", p.Clock()))) + assert.NoError(t, err) + + // Diff sync carries the new object (current value) to target. + _ = testutils.SyncData(p, target) // returns io.EOF on normal completion + + assertResolves(t, target, cid, rid, "diff-sync new object must index current value v2") +} + +// Many new objects arrive in one diff into an existing-class replica (reindex OFF). +// Validates that classCache resolves every object at scale (O/fields contiguous). +func TestIndexConverge_DiffSyncManyObjects(t *testing.T) { + dirs, clear := testdirs(0xa, 0xb) + defer clear() + ctx := context.Background() + + p := openIdx(t, dirs[0], 0xa) + defer p.Close() + target := openIdx(t, dirs[1], 0xb) + defer target.Close() + + cid, err := p.NewClass(ctx, rdx.ID0, SchemaIndex...) + assert.NoError(t, err) + _ = testutils.SyncData(p, target) // target gets the class only + + const n = 100 + rids := make(map[string]rdx.ID, n) + for i := 0; i < n; i++ { + val := fmt.Sprintf("obj%d", i) + rid, err := p.NewObjectTLV(ctx, cid, protocol.Records{protocol.Record('S', rdx.Stlv(val))}) + assert.NoError(t, err) + rids[val] = rid + } + // SyncData's short WaitUntilNone can transfer only part of a large diff per + // call (esp. under -race); it's state-based, so repeat until all data lands. + present := func() (c int) { + for _, rid := range rids { + if _, _, err := target.ObjectFieldTLV(rid.ToOff(1)); err == nil { + c++ + } + } + return + } + for attempt := 0; attempt < 30 && present() < n; attempt++ { + _ = testutils.SyncData(p, target) + } + assert.Equal(t, n, present(), "all objects must reach target") + + for val, rid := range rids { + got, err := target.ObjectIDByHash(cid, 1, []byte(val)) + assert.NoError(t, err, "diff-sync must index %s", val) + assert.Equal(t, rid, got, "diff-sync must resolve %s to its object", val) + } +} + +// A diff sync that EDITS a pre-existing object's indexed field must move the +// index to the new value (and stop resolving the old one). Reindex OFF. +func TestIndexConverge_DiffSyncEditExisting(t *testing.T) { + dirs, clear := testdirs(0xa, 0xb) + defer clear() + ctx := context.Background() + + p := openIdx(t, dirs[0], 0xa) + defer p.Close() + target := openIdx(t, dirs[1], 0xb) + defer target.Close() + + cid, err := p.NewClass(ctx, rdx.ID0, SchemaIndex...) + assert.NoError(t, err) + rid, err := p.NewObjectTLV(ctx, cid, protocol.Records{protocol.Record('S', rdx.Stlv("v1"))}) + assert.NoError(t, err) + _ = testutils.SyncData(p, target) // target has the object, indexed as "v1" + + got, err := target.ObjectIDByHash(cid, 1, []byte("v1")) + assert.NoError(t, err, "pre-edit: v1 resolves") + assert.Equal(t, rid, got) + + // Edit the indexed field on p, then sync the edit. + _, oldTlv, err := p.ObjectFieldTLV(rid.ToOff(1)) + assert.NoError(t, err) + _, err = p.EditFieldTLV(ctx, rid.ToOff(1), protocol.Record('S', rdx.Sdelta(oldTlv, "v2", p.Clock()))) + assert.NoError(t, err) + _ = testutils.SyncData(p, target) + + got, err = target.ObjectIDByHash(cid, 1, []byte("v2")) + assert.NoError(t, err, "post-edit: v2 must resolve") + assert.Equal(t, rid, got) + // Note: a lookup of the OLD value "v1" may still resolve here because + // hashIndexCache positive-caches value->rid and GetByHash skips revalidation + // on a cache hit. That's a separate, pre-existing cache concern (self-limiting + // via LRU), not part of the index-convergence guarantee under test. +} + +// O then E, in order, separate drains — the control case (already correct on main). +func TestIndexConverge_LiveObeforeE(t *testing.T) { + dirs, clear := testdirs(0xa, 0xb) + defer clear() + ctx := context.Background() + + p := openIdx(t, dirs[0], 0xa) + defer p.Close() + cid, rid, cRecs, oRecs, eRecs := liveUnits(t, p) + + target := openIdx(t, dirs[1], 0xb) + defer target.Close() + assert.NoError(t, target.Drain(ctx, cRecs)) + assert.NoError(t, target.Drain(ctx, oRecs)) + assert.NoError(t, target.Drain(ctx, eRecs)) + + assertResolves(t, target, cid, rid, "O-before-E must index current value v2") +} diff --git a/chotki_index_eo_test.go b/chotki_index_eo_test.go new file mode 100644 index 0000000..6ecf86e --- /dev/null +++ b/chotki_index_eo_test.go @@ -0,0 +1,97 @@ +package chotki + +import ( + "context" + "sync" + "testing" + "time" + + "github.com/drpcorg/chotki/protocol" + "github.com/drpcorg/chotki/rdx" + "github.com/drpcorg/chotki/replication" + "github.com/drpcorg/chotki/utils" + "github.com/stretchr/testify/require" +) + +// Concurrent drains are not serialized (Drain holds only an RLock), so an 'E' +// editing a hash-indexed field can race the 'O' creating its object on another +// session. Pre-fix, the E's index update was silently skipped when the object +// was not yet visible (class unresolvable -> ID0 -> "not indexed"), and if the +// O's post-commit reindex also ran before the E committed, the index stayed +// stale forever (no reindex task is scheduled for that case). Now ApplyE +// defers such edits to the same post-commit IndexObject pass as created +// objects: whichever of E/O commits last reindexes from the merged state. +func TestIndexConverge_ConcurrentEO(t *testing.T) { + dirs, clear := testdirs(0xa) + defer clear() + + a, err := Open(dirs[0], Options{Src: 0xa, Name: "replica A"}) + require.NoError(t, err) + defer a.Close() + + // capture the C, O and E packets off replica A + q := utils.NewFDQueue[protocol.Records](1<<20, 50*time.Millisecond, 1) + a.outq.Store("capture", q) + defer a.outq.Delete("capture") + + cid, err := a.NewClass(context.Background(), rdx.ID0, SchemaIndex...) + require.NoError(t, err) + aorm := a.ObjectMapper() + defer aorm.Close() + obj := Test{Test: "v1"} + require.NoError(t, aorm.New(context.Background(), cid, &obj)) + aorm.UpdateAll() + obj.Test = "v2" + require.NoError(t, aorm.Save(context.Background(), &obj)) + aorm.UpdateAll() + + packets := drainQueue(t, q, 3, 5*time.Second) + require.Len(t, packets, 3) + var cPack, oPack, ePack []byte + for _, p := range packets { + lit, _, _, _, perr := replication.ParsePacket(p) + require.NoError(t, perr) + switch lit { + case 'C': + cPack = p + case 'O': + oPack = p + case 'E': + ePack = p + } + } + require.NotNil(t, cPack) + require.NotNil(t, oPack) + require.NotNil(t, ePack) + + // replay the O and E concurrently into fresh replicas; the hash index + // must always converge to the edited value + for round := 0; round < 8; round++ { + cdirs, cclear := testdirs(0xc) + c, err := Open(cdirs[0], Options{Src: 0xc, Name: "replica C"}) + require.NoError(t, err) + + require.NoError(t, c.Drain(context.Background(), protocol.Records{cPack})) + + var wg sync.WaitGroup + wg.Add(2) + go func() { + defer wg.Done() + _ = c.Drain(context.Background(), protocol.Records{oPack}) + }() + go func() { + defer wg.Done() + _ = c.Drain(context.Background(), protocol.Records{ePack}) + }() + wg.Wait() + + corm := c.ObjectMapper() + got, err := waitGetByHash[*Test](t, corm, cid, 1, []byte("v2"), 10*time.Second) + require.NoError(t, err, "round %d: hash index must converge to the edited value", round) + require.Equal(t, "v2", got.Test, "round %d", round) + + require.NoError(t, corm.Close()) + require.NoError(t, c.Close()) + cclear() + } +} diff --git a/chotki_index_test.go b/chotki_index_test.go index 68d08b0..0cab6ec 100644 --- a/chotki_index_test.go +++ b/chotki_index_test.go @@ -13,41 +13,47 @@ import ( testutils "github.com/drpcorg/chotki/test_utils" "github.com/drpcorg/chotki/utils" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) var SchemaIndex = []classes.Field{ {Name: "test", RdxType: rdx.String, Index: classes.HashIndex}, } -// waitGetByHash polls GetByHash until it finds the object or the timeout -// expires. Hash indexes on the receiving replica can be populated -// asynchronously by the reindex worker (see IndexManager.CheckReindexTasks), -// so a fresh read right after SyncData may legitimately miss the index on -// slower machines. orm.Clear() is called on every attempt to refresh the -// snapshot the ORM reads through. +// waitGetByHash waits (require.Eventually) until GetByHash finds the object. +// Hash indexes on the receiving replica can be populated asynchronously by the +// reindex worker (see IndexManager.CheckReindexTasks), so a fresh read right +// after SyncData may legitimately miss the index on slower machines. +// orm.Clear() is called on every attempt to refresh the snapshot the ORM reads +// through. Fails the test on timeout or on any error other than +// ErrObjectUnknown. func waitGetByHash[T NativeObject](t *testing.T, orm *ORM, cid rdx.ID, fid uint32, tlv []byte, timeout time.Duration) (T, error) { t.Helper() - deadline := time.Now().Add(timeout) var ( obj T err error ) - for { + require.Eventually(t, func() bool { if cerr := orm.Clear(); cerr != nil { - return obj, cerr + err = cerr + return true // non-retriable: bail out and let the caller assert } obj, err = GetByHash[T](orm, cid, fid, tlv) if err == nil { - return obj, nil + return true } - if !errors.Is(err, chotki_errors.ErrObjectUnknown) { - return obj, err - } - if time.Now().After(deadline) { - return obj, err - } - time.Sleep(25 * time.Millisecond) - } + return !errors.Is(err, chotki_errors.ErrObjectUnknown) // retry only unknown-object misses + }, timeout, 25*time.Millisecond, "hash index for %q not populated in %v", string(tlv), timeout) + return obj, err +} + +// waitLiveSession waits until the replica has at least one live replication +// session (its broadcast queue is registered in outq). Replaces fixed sleeps +// after net.Listen/Connect. +func waitLiveSession(t *testing.T, cho *Chotki) { + t.Helper() + require.Eventually(t, func() bool { return cho.outq.Size() > 0 }, + 10*time.Second, 10*time.Millisecond, "live sync session did not establish") } func TestFullScanIndexSync(t *testing.T) { @@ -104,19 +110,20 @@ func TestFullScanIndexSync(t *testing.T) { err = b.net.Connect("tcp://127.0.0.1:34934") assert.NoError(t, err) - time.Sleep(time.Second * 1) + waitLiveSession(t, b) ob2 := Test{Test: "test2"} err = borm.New(context.Background(), cid, &ob2) assert.NoError(t, err) borm.UpdateAll() - time.Sleep(time.Millisecond * 100) - - data = make([]Test, 0) - for item := range SeekClass[*Test](borm, cid) { - data = append(data, *item) - } + require.Eventually(t, func() bool { + data = make([]Test, 0) + for item := range SeekClass[*Test](borm, cid) { + data = append(data, *item) + } + return len(data) == 2 + }, 10*time.Second, 25*time.Millisecond, "full-scan index did not converge after live sync") assert.Equal(t, []Test{{Test: "test1"}, {Test: "test2"}}, data, "index in sync check after live sync") } @@ -167,7 +174,7 @@ func TestHashIndexSyncCreateObject(t *testing.T) { borm := b.ObjectMapper() defer borm.Close() - test1data, err = waitGetByHash[*Test](t, borm, cid, 1, []byte("test1"), 5*time.Second) + test1data, err = waitGetByHash[*Test](t, borm, cid, 1, []byte("test1"), 30*time.Second) assert.NoError(t, err) assert.Equal(t, &Test{Test: "test1"}, test1data, "index in sync check after diff sync") @@ -177,14 +184,14 @@ func TestHashIndexSyncCreateObject(t *testing.T) { err = b.net.Connect("tcp://127.0.0.1:34934") assert.NoError(t, err) - time.Sleep(time.Second * 1) + waitLiveSession(t, b) ob2 := Test{Test: "test2"} err = borm.New(context.Background(), cid, &ob2) assert.NoError(t, err) borm.UpdateAll() - test2data, err := waitGetByHash[*Test](t, aorm, cid, 1, []byte("test2"), 5*time.Second) + test2data, err := waitGetByHash[*Test](t, aorm, cid, 1, []byte("test2"), 30*time.Second) assert.NoError(t, err) assert.Equal(t, &Test{Test: "test2"}, test2data, "index in sync check after live sync") } @@ -247,7 +254,7 @@ func TestHashIndexSyncEditObject(t *testing.T) { borm := b.ObjectMapper() defer borm.Close() - test1data, err = waitGetByHash[*Test](t, borm, cid, 1, []byte("test10"), 5*time.Second) + test1data, err = waitGetByHash[*Test](t, borm, cid, 1, []byte("test10"), 30*time.Second) assert.NoError(t, err) assert.Equal(t, &Test{Test: "test10"}, test1data, "index in sync check after diff sync") @@ -257,13 +264,13 @@ func TestHashIndexSyncEditObject(t *testing.T) { err = b.net.Connect("tcp://127.0.0.1:34934") assert.NoError(t, err) - time.Sleep(time.Second * 1) + waitLiveSession(t, b) test1data.Test = "test11" borm.Save(context.Background(), test1data) borm.UpdateAll() - test1data, err = waitGetByHash[*Test](t, aorm, cid, 1, []byte("test11"), 5*time.Second) + test1data, err = waitGetByHash[*Test](t, aorm, cid, 1, []byte("test11"), 30*time.Second) assert.NoError(t, err) assert.Equal(t, &Test{Test: "test11"}, test1data, "index in sync check after live sync") } @@ -307,7 +314,7 @@ func TestHashIndexRepairIndex(t *testing.T) { borm := b.ObjectMapper() defer borm.Close() - test1data, err := waitGetByHash[*Test](t, borm, cid, 1, []byte("test1"), 5*time.Second) + test1data, err := waitGetByHash[*Test](t, borm, cid, 1, []byte("test1"), 30*time.Second) assert.NoError(t, err) assert.Equal(t, &Test{Test: "test1"}, test1data, "index in sync check after diff sync") } diff --git a/chotki_index_unique_batch_test.go b/chotki_index_unique_batch_test.go new file mode 100644 index 0000000..888c6c7 --- /dev/null +++ b/chotki_index_unique_batch_test.go @@ -0,0 +1,60 @@ +package chotki + +import ( + "context" + "testing" + + "github.com/drpcorg/chotki/chotki_errors" + "github.com/drpcorg/chotki/classes" + "github.com/drpcorg/chotki/host" + "github.com/drpcorg/chotki/protocol" + "github.com/drpcorg/chotki/rdx" + "github.com/stretchr/testify/require" +) + +// Two edits in one CommitBatch setting the same unique hash-indexed value on +// different objects must be rejected before the batch commits. The +// uniqueness gate reads committed state, so without the per-drain claims map the +// two writes never see each other and both land, producing a forbidden multi-id +// index set. +func TestHashUniqueViolationWithinBatch(t *testing.T) { + dirs, clear := testdirs(0x7c) + defer clear() + a, err := Open(dirs[0], Options{Src: 0x7c, Name: "A"}) + require.NoError(t, err) + defer a.Close() + + cid, err := a.NewClass(context.Background(), rdx.ID0, + classes.Field{Name: "u", RdxType: rdx.String, Index: classes.HashIndex}) + require.NoError(t, err) + + o1, err := a.NewObjectTLV(context.Background(), cid, protocol.Records{ + protocol.Record(rdx.String, rdx.Stlv("alice")), + }) + require.NoError(t, err) + o2, err := a.NewObjectTLV(context.Background(), cid, protocol.Records{ + protocol.Record(rdx.String, rdx.Stlv("bob")), + }) + require.NoError(t, err) + + vvBefore, err := a.VersionVector() + require.NoError(t, err) + + // one batch sets BOTH objects' unique field to the same value + dupEdit := func(oid rdx.ID) host.Edit { + return host.Edit{Ref: oid, Body: protocol.Records{ + protocol.Record('F', rdx.ZipUint64(1)), + protocol.Record(rdx.String, rdx.Stlv("carol")), + }} + } + err = a.CommitBatch(context.Background(), []host.Edit{dupEdit(o1), dupEdit(o2)}) + require.ErrorIs(t, err, chotki_errors.ErrHashIndexUinqueConstraintViolation, + "a duplicate unique value within one batch must be rejected") + + // drop-on-error: the rejected batch persists nothing, so the version vector + // did not advance. + vvAfter, err := a.VersionVector() + require.NoError(t, err) + require.Equal(t, vvBefore.GetID(0x7c), vvAfter.GetID(0x7c), + "a rejected batch must persist nothing") +} diff --git a/chotki_options_test.go b/chotki_options_test.go new file mode 100644 index 0000000..e2a0cc7 --- /dev/null +++ b/chotki_options_test.go @@ -0,0 +1,25 @@ +package chotki + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +func TestCounterSyncPeriodDefault(t *testing.T) { + o := Options{} + o.SetDefaults() + assert.Equal(t, time.Second, o.CounterSyncPeriod) + + o2 := Options{CounterSyncPeriod: 5 * time.Minute} + o2.SetDefaults() + assert.Equal(t, 5*time.Minute, o2.CounterSyncPeriod, "explicit value must be preserved") + + // A negative period must be normalized, not left to silently disable the + // background flush. + o3 := Options{CounterSyncPeriod: -1} + o3.SetDefaults() + assert.Greater(t, o3.CounterSyncPeriod, time.Duration(0), + "a non-positive CounterSyncPeriod must be normalized to the default") +} diff --git a/chotki_reindex_close_test.go b/chotki_reindex_close_test.go new file mode 100644 index 0000000..209cf2e --- /dev/null +++ b/chotki_reindex_close_test.go @@ -0,0 +1,57 @@ +package chotki + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/drpcorg/chotki/rdx" + testutils "github.com/drpcorg/chotki/test_utils" + "github.com/stretchr/testify/require" +) + +// The reindex worker (CheckReindexTasks) spawns runReindexTask goroutines. +// They used to be untracked: Close cancelled the context and waited only for +// the worker itself, then closed pebble — a task still iterating/merging would +// panic ("pebble: closed"), crashing the whole test binary intermittently +// under load. Now the worker waits for its spawned tasks (taskWg) before +// returning, so Close never yanks the DB from under a running reindex. +func TestCloseWaitsForRunningReindexTasks(t *testing.T) { + reindexPeriod = time.Millisecond + t.Cleanup(func() { reindexPeriod = time.Second }) + + for i := 0; i < 5; i++ { + dirs, clear := testdirs(0xa, 0xb) + + a, err := Open(dirs[0], Options{Src: 0xa, Name: "replica A", + ReadAccumTimeLimit: 100 * time.Millisecond}) + require.NoError(t, err) + + b, err := Open(dirs[1], Options{Src: 0xb, Name: "replica B", + ReadAccumTimeLimit: 100 * time.Millisecond}) + require.NoError(t, err) + + cid, err := a.NewClass(context.Background(), rdx.ID0, SchemaIndex...) + require.NoError(t, err) + + // enough hash-indexed objects that the receiver's reindex task has + // real work in flight when we close it + aorm := a.ObjectMapper() + for j := 0; j < 64; j++ { + obj := Test{Test: fmt.Sprintf("test-%d-%d", i, j)} + require.NoError(t, aorm.New(context.Background(), cid, &obj)) + } + aorm.UpdateAll() + + // the diff sync hands b's index work to its reindex tasks + testutils.SyncData(a, b) + + // close immediately: pre-fix, a spawned runReindexTask could still be + // mid-Merge here and panic on the closed DB + require.NoError(t, aorm.Close()) + require.NoError(t, b.Close()) + require.NoError(t, a.Close()) + clear() + } +} diff --git a/chotki_snapshot_restore_test.go b/chotki_snapshot_restore_test.go new file mode 100644 index 0000000..d781592 --- /dev/null +++ b/chotki_snapshot_restore_test.go @@ -0,0 +1,62 @@ +package chotki + +import ( + "context" + "testing" + + "github.com/drpcorg/chotki/protocol" + "github.com/drpcorg/chotki/rdx" + "github.com/stretchr/testify/require" +) + +// After a diff-sync delivers our OWN history back to us — as happens when a +// replica restores from an older snapshot and a peer replays its tail — the +// synced version vector covers cho.src beyond the local allocator. cho.last +// must advance to that VV, or the next local commit reissues an id a peer +// already holds: two different mutations under one rdx.ID, cluster-wide +// divergence. The own history arrives via the handshake VV / diff +// batch, NOT as top-level own-source packets, so the maxOwn path (covered by +// TestCommitAllocatorSyncedWithOwnSourceDrain) never sees it — only the +// advance-from-merged-VV at 'V' closes the window. +func TestAllocatorAdvancesAfterOwnHistoryRestoredViaSync(t *testing.T) { + dirs, clear := testdirs(0x5c) + defer clear() + a, err := Open(dirs[0], Options{Src: 0x5c, Name: "replica A"}) + require.NoError(t, err) + defer a.Close() + + // A has only a little own history so far. + _, err = a.NewClass(context.Background(), rdx.ID0, Schema...) + require.NoError(t, err) + before := a.Last() + + // A peer (0x99) that holds our (0x5c) history up to seq 100 hands it back via + // a diff-sync. Its handshake VV claims 0x5c@100; ApplyH stages that VV into + // the sync batch, which becomes durable at 'V'. + ownHigh := rdx.IDFromSrcSeqOff(0x5c, 100, 0) + require.True(t, before.Less(ownHigh)) + + hid := rdx.IDFromSrcSeqOff(0x99, 7, 0) + // VV values are pro, not seq — build via PutID so the encoded ids are exact. + peerVV := rdx.VV{} + peerVV.PutID(hid) + peerVV.PutID(ownHigh) + hpack := protocol.Record('H', + protocol.TinyRecord('T', hid.ZipBytes()), + protocol.Record('M', []byte{0}), + protocol.Record('V', peerVV.TLV()), + ) + // an empty diff closed immediately by its 'V' (no D-parcels needed to + // exercise the allocator advance) + vpack := protocol.Record('V', protocol.TinyRecord('T', hid.ZipBytes())) + + err = a.Drain(context.Background(), protocol.Records{hpack, vpack}) + require.NoError(t, err) + + vv, err := a.VersionVector() + require.NoError(t, err) + require.Equal(t, vv.GetID(0x5c), a.Last(), + "cho.last must equal the synced VV[src] after own history is restored") + require.False(t, a.Last().Less(ownHigh), + "the reissue window is closed only if cho.last covers the restored history") +} diff --git a/chotki_sync_regression_test.go b/chotki_sync_regression_test.go new file mode 100644 index 0000000..cd40555 --- /dev/null +++ b/chotki_sync_regression_test.go @@ -0,0 +1,345 @@ +package chotki + +import ( + "context" + "io" + "log/slog" + "testing" + "time" + + "github.com/drpcorg/chotki/protocol" + "github.com/drpcorg/chotki/rdx" + "github.com/drpcorg/chotki/replication" + "github.com/drpcorg/chotki/utils" + "github.com/stretchr/testify/require" +) + +// Regression tests for the replication protocol bugs found while writing +// the TLA+ spec of the sync protocol (see tla/README.md for the full +// stories). Each test fails against the pre-fix behaviour. + +func newSyncer(host *Chotki, name string) *replication.Syncer { + return &replication.Syncer{ + Src: host.Source(), + Host: host, + Mode: replication.SyncRWLive, + Name: name, + Log: utils.NewDefaultLogger(slog.LevelError), + PingWait: time.Second, + PingPeriod: time.Minute, + WaitUntilNone: time.Millisecond, + } +} + +func byeRecord(snaplast rdx.ID) []byte { + return protocol.Record('B', + protocol.TinyRecord('T', snaplast.ZipBytes()), + []byte("closing")) +} + +func syncPointKeys(cho *Chotki) map[rdx.ID]bool { + keys := make(map[rdx.ID]bool) + cho.syncs.Range(func(key rdx.ID, _ *syncPoint) bool { + keys[key] = true + return true + }) + return keys +} + +// drainQueue reads records from an FDQueue until it has at least want +// records or the deadline passes. +func drainQueue(t *testing.T, q *utils.FDQueue[protocol.Records], want int, timeout time.Duration) protocol.Records { + t.Helper() + var got protocol.Records + deadline := time.Now().Add(timeout) + for len(got) < want && time.Now().Before(deadline) { + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + recs, _ := q.Feed(ctx) + cancel() + got = append(got, recs...) + } + return got +} + +// Bug 1 (tla/README.md, MCBuggyRelay.cfg): a drained batch whose last +// record is 'B' (bye) — which network read batching routinely coalesces +// with the preceding data records — must still rebroadcast the applied +// records to the other sessions, minus the session-scoped trailing 'B'. +// Pre-fix, the whole batch was silently dropped from the relay, leaving +// downstream replicas permanently diverged. +func TestDrainRelaysAppliedRecordsWhenBatchEndsWithBye(t *testing.T) { + dirs, clear := testdirs(0x51, 0x52) + defer clear() + + a, err := Open(dirs[0], Options{Src: 0x51, Name: "replica A"}) + require.NoError(t, err) + defer a.Close() + b, err := Open(dirs[1], Options{Src: 0x52, Name: "replica B"}) + require.NoError(t, err) + defer b.Close() + + // capture the packet replica A commits (as its live broadcast) + qa := utils.NewFDQueue[protocol.Records](1<<20, 50*time.Millisecond, 1) + a.outq.Store("capture", qa) + cid, err := a.NewClass(context.Background(), rdx.ID0, Schema...) + require.NoError(t, err) + packets := drainQueue(t, qa, 1, 2*time.Second) + require.Len(t, packets, 1) + require.Equal(t, byte('C'), protocol.Lit(packets[0])) + + // replica B has a downstream session "c" that must receive relays + qc := utils.NewFDQueue[protocol.Records](1<<20, 50*time.Millisecond, 1) + b.outq.Store("c", qc) + + // the a—b session at B: handshake, then a batch of [C-packet, B(bye)] + // coalesced the way network/peer.go keepRead does it + sa, sb := newSyncer(a, "b"), newSyncer(b, "a") + defer sa.Close() + defer sb.Close() + ha, err := sa.FeedHandshake() + require.NoError(t, err) + require.NoError(t, sb.Drain(context.Background(), ha)) + require.NoError(t, sb.Drain(context.Background(), + protocol.Records{packets[0], byeRecord(a.Last())})) + + // B applied the class locally... + _, err = b.ClassFields(cid) + require.NoError(t, err) + + // ...and must have relayed the handshake and the class packet (but + // not the bye) downstream + relayed := drainQueue(t, qc, 2, 2*time.Second) + require.Len(t, relayed, 2) + require.Equal(t, byte('H'), protocol.Lit(relayed[0])) + require.Equal(t, byte('C'), protocol.Lit(relayed[1])) + for _, rec := range relayed { + require.NotEqual(t, byte('B'), protocol.Lit(rec), "session-scoped bye must not be relayed") + } +} + +// Bug 2 (MCBuggyDisplace.cfg): a new handshake from a replica must +// displace that replica's older, unfinished sync points ("allow only 1 +// diff sync per src"). Pre-fix the cleanup compared keys against the +// local src, which never matches, so stale sync points were never +// displaced. +func TestHandshakeDisplacesStaleSyncPointOfSameSrc(t *testing.T) { + dirs, clear := testdirs(0x53, 0x54) + defer clear() + + a, err := Open(dirs[0], Options{Src: 0x53, Name: "replica A"}) + require.NoError(t, err) + defer a.Close() + b, err := Open(dirs[1], Options{Src: 0x54, Name: "replica B"}) + require.NoError(t, err) + defer b.Close() + + _, err = a.NewClass(context.Background(), rdx.ID0, Schema...) + require.NoError(t, err) + + // session 1 from A dies mid-diff, leaving a stale sync point at B + sa1, sb1 := newSyncer(a, "b"), newSyncer(b, "a") + h1, err := sa1.FeedHandshake() + require.NoError(t, err) + k1 := a.Last() + require.NoError(t, sb1.Drain(context.Background(), h1)) + require.True(t, syncPointKeys(b)[k1], "sync point for session 1 must exist") + + // A commits more (so its snapshot id changes) and re-handshakes + _, err = a.NewClass(context.Background(), rdx.ID0, Schema...) + require.NoError(t, err) + sa2, sb2 := newSyncer(a, "b"), newSyncer(b, "a") + defer sa2.Close() + defer sb2.Close() + h2, err := sa2.FeedHandshake() + require.NoError(t, err) + k2 := a.Last() + require.NotEqual(t, k1, k2) + require.NoError(t, sb2.Drain(context.Background(), h2)) + + keys := syncPointKeys(b) + require.True(t, keys[k2], "sync point for session 2 must exist") + require.False(t, keys[k1], "stale sync point of the same src must be displaced") +} + +// Bug 3 (MCBuggyStaleSync.cfg, found by TLC): a sync point must not +// outlive the session whose 'H' created it — the remaining 'D'/'V' +// records travel through that session's connection, so once it is gone +// the staged batch can never be completed legitimately, while a 'V' +// arriving through a newer connection would apply the staged handshake +// VV without the data (permanent silent data loss). +func TestSyncerCloseAbortsItsSyncPoints(t *testing.T) { + dirs, clear := testdirs(0x55, 0x56) + defer clear() + + a, err := Open(dirs[0], Options{Src: 0x55, Name: "replica A"}) + require.NoError(t, err) + defer a.Close() + b, err := Open(dirs[1], Options{Src: 0x56, Name: "replica B"}) + require.NoError(t, err) + defer b.Close() + + _, err = a.NewClass(context.Background(), rdx.ID0, Schema...) + require.NoError(t, err) + + sa, sb := newSyncer(a, "b"), newSyncer(b, "a") + defer sa.Close() + h, err := sa.FeedHandshake() + require.NoError(t, err) + k := a.Last() + require.NoError(t, sb.Drain(context.Background(), h)) + require.True(t, syncPointKeys(b)[k]) + + // the session dies mid-diff: its sync point must die with it + require.NoError(t, sb.Close()) + require.False(t, syncPointKeys(b)[k], "sync point must be aborted with its session") + + // a late 'V' for that diff (e.g. relayed through a newer connection) + // must fail loudly instead of poisoning the version vector + _, vpack := protocol.OpenHeader(nil, 'V') + vpack = append(vpack, protocol.Record('T', k.ZipBytes())...) + protocol.CloseHeader(vpack, 5) + err = b.Drain(context.Background(), protocol.Records{vpack}) + require.ErrorIs(t, err, ErrSyncUnknown) +} + +// Bug 4 (tla/README.md item 11): a peer's bye only means it has nothing +// more to send; its drain side keeps applying records until the +// connection closes. The feed must therefore finish its own diff phase +// (through 'V') instead of cutting it short, which would strand a staged +// diff batch at the peer that silently misses the promised data. +func TestFeedFinishesDiffAfterPeerBye(t *testing.T) { + dirs, clear := testdirs(0x57, 0x58) + defer clear() + + a, err := Open(dirs[0], Options{Src: 0x57, Name: "replica A"}) + require.NoError(t, err) + defer a.Close() + b, err := Open(dirs[1], Options{Src: 0x58, Name: "replica B"}) + require.NoError(t, err) + defer b.Close() + + cid, err := a.NewClass(context.Background(), rdx.ID0, Schema...) + require.NoError(t, err) + + sa, sb := newSyncer(a, "b"), newSyncer(b, "a") + defer sa.Close() + defer sb.Close() + + // exchange handshakes + ha, err := sa.Feed(context.Background()) + require.NoError(t, err) + require.NoError(t, sb.Drain(context.Background(), ha)) + hb, err := sb.Feed(context.Background()) + require.NoError(t, err) + require.NoError(t, sa.Drain(context.Background(), hb)) + + // B (an empty, fast peer) says bye before A has fed its diff + require.NoError(t, sa.Drain(context.Background(), + protocol.Records{byeRecord(b.Last())})) + + // A must still deliver its complete diff, V packet included + sawV := false + for { + recs, err := sa.Feed(context.Background()) + if err == io.EOF { + break + } + require.NoError(t, err) + for _, rec := range recs { + if protocol.Lit(rec) == 'V' { + sawV = true + } + } + require.NoError(t, sb.Drain(context.Background(), recs)) + } + require.True(t, sawV, "feed must finish the diff (send 'V') after a peer bye") + + // and B must actually have the data A promised in its handshake + _, err = b.ClassFields(cid) + require.NoError(t, err, "peer must not be left without the promised diff data") +} + +// cho.last (the cached local id allocator read by CommitPacket) is also +// advanced by replication sessions draining records stamped with our own +// src — e.g. our own history synced back after a restore from an older +// snapshot. Those two paths do not share a lock and used to race on the +// unsynchronized cho.last, letting a commit reuse an already issued seq: +// two different mutations under one rdx.ID (tla/README.md, bug 4; +// MCBuggyLast.cfg is the TLA+ witness). +func TestCommitAllocatorSyncedWithOwnSourceDrain(t *testing.T) { + dirs, clear := testdirs(0x5a) + defer clear() + + a, err := Open(dirs[0], Options{Src: 0x5a, Name: "replica A"}) + require.NoError(t, err) + defer a.Close() + + // capture a real 'C' packet to reuse its class body for the + // hand-crafted own-source records below + qa := utils.NewFDQueue[protocol.Records](1<<20, 50*time.Millisecond, 1) + a.outq.Store("capture", qa) + _, err = a.NewClass(context.Background(), rdx.ID0, Schema...) + require.NoError(t, err) + packets := drainQueue(t, qa, 1, 2*time.Second) + require.Len(t, packets, 1) + a.outq.Delete("capture") + _, _, ref, body, err := replication.ParsePacket(packets[0]) + require.NoError(t, err) + + ownSrcPacket := func(seq uint64) protocol.Records { + id := rdx.IDFromSrcSeqOff(0x5a, seq, 0) + return protocol.Records{protocol.Record('C', + protocol.Record('I', id.ZipBytes()), + protocol.Record('R', ref.ZipBytes()), + body)} + } + + // a replication session feeding our own records back, racing local + // commits (run the test with -race to catch the unsynchronized + // cho.last accesses) + done := make(chan struct{}) + go func() { + defer close(done) + for i := uint64(0); i < 50; i++ { + _ = a.Drain(context.Background(), ownSrcPacket(1000+i*100)) + } + }() + + seen := make(map[rdx.ID]bool) + for i := 0; i < 50; i++ { + cid, err := a.NewClass(context.Background(), rdx.ID0, Schema...) + require.NoError(t, err) + require.False(t, seen[cid], "commit reused an already issued id %s", cid) + seen[cid] = true + } + <-done + + // the allocator must have caught up with the drained history: the + // next commit must not reuse any of those seqs either + maxDrained := rdx.IDFromSrcSeqOff(0x5a, 1000+49*100, 0) + require.False(t, a.Last().Less(maxDrained)) + cid, err := a.NewClass(context.Background(), rdx.ID0, Schema...) + require.NoError(t, err) + require.True(t, maxDrained.Less(cid)) +} + +// ApplyD/ApplyV used to loop forever on truncated inner TLV records +// (protocol.Take makes no progress on incomplete input) and ApplyV could +// overwrite an earlier ErrBadVPacket with a later successful merge. +func TestApplyDVMalformedInputFailsFast(t *testing.T) { + dirs, clear := testdirs(0x59) + defer clear() + + a, err := Open(dirs[0], Options{Src: 0x59, Name: "replica A"}) + require.NoError(t, err) + defer a.Close() + + batch := a.Database().NewBatch() + defer batch.Close() + + // short-form headers claiming 200-byte bodies that are not there + err = a.ApplyV(rdx.ID0, rdx.ID0, []byte{'v', 200}, batch) + require.ErrorIs(t, err, ErrBadVPacket) + err = a.ApplyD(rdx.ID0, rdx.ID0, []byte{'f', 200}, batch, nil, nil) + require.ErrorIs(t, err, rdx.ErrBadPacket) +} diff --git a/counters/README.md b/counters/README.md index 19ebf0e..96c068d 100644 --- a/counters/README.md +++ b/counters/README.md @@ -1,59 +1,46 @@ -# AtomicCounter Architecture +# AtomicCounter & AtomicCounterManager -AtomicCounter provides atomic increment operations in distributed environments while -optimizing performance through intelligent caching. It supports Natural (increment-only) -and ZCounter (two-way) types with CRDT merge semantics. +`AtomicCounter` is a **lock-free** CRDT counter (Natural increment-only or ZCounter +two-way). `Get`/`Increment` touch only in-memory atomics; an `AtomicCounterManager` +background goroutine periodically persists the local contribution and reloads other +replicas' contributions. -## Core Design Principle +## Model -The counter trades CPU usage for data freshness, but **only for data from other replicas**. -All writes to the current replica are immediately reflected in the counter value. -Caching only affects how frequently we read data that arrived via synchronization. +- Each field tracks `mine` (this replica's contribution, `atomic.Int64`) and `theirs` + (the last-loaded sum of all other replicas, `atomic.Int64`). +- `Get()` returns `mine + theirs`. +- `Increment(v)` adds to `mine` (Natural rejects `v < 0`). +- The background goroutine, every `Options.CounterSyncPeriod`, **flushes** all changed fields + in **batched commits** — up to 1024 fields per Pebble batch + broadcast — and **reloads** + `theirs` for fields touched since the last tick (idle fields cost nothing). A flush is a + read-modify-write under the host's sequential-write bracket: it reads the current own-src + slot and commits slot + unflushed delta (never the absolute cached `mine`), so a concurrent + writer to the same slot (e.g. an ORM counter "set") is not clobbered and the merge cannot + silently drop either write. -## How It Works +Local increments are visible immediately via `Get`. Other replicas' increments become +visible after the next reload. Obtain a counter with `cho.Counter(rid, offset)`. -The counter uses a lazy loading pattern with time-based caching. When data is requested: +## Durability tradeoff -1. **Cache Check**: If cached data hasn't expired, return it immediately -2. **Database Load**: Otherwise, load fresh data from the LSM database -3. **Parse & Cache**: Parse TLV data into internal structures and cache with expiration +Increments live in memory between flushes. A **graceful** `Close()` flushes everything; a +**hard crash** (panic / kill -9 / power loss) loses increments since the last flush. This is +the deliberate tradeoff for a lock-free hot path. Absent a crash, **no event is missed**: +the unflushed delta (`mine - lastSynced`) survives a skipped, coalesced or failed flush and +is carried into the next one. -For increments, the process is: +## Forcing a cycle -1. **Load Data**: Get current counter state (cached or from DB) -2. **Atomic Update**: Use Go's atomic primitives to update the value -3. **Generate TLV**: Create TLV records for persistence -4. **Commit**: Write changes to database with CRDT merge semantics +`cho.SyncCounters(ctx)` runs one flush + full reload synchronously (used in tests and when a +caller wants an immediate, complete sync). The background ticker uses the same machinery but +only reloads fields touched since the previous tick (and retries any that failed to load). -## Internal Structure - -The counter maintains two internal representations: - -- **atomicNcounter**: For Natural counters, uses atomic.Uint64 for thread-safe increments -- **atomicZCounter**: For ZCounter, uses atomic.Pointer with revision tracking for conflict resolution - -## Performance Trade-offs - -The design trades CPU usage for freshness of **synchronized data from other replicas**. -With updatePeriod > 0, the counter caches data to avoid expensive database reads, -but may return slightly stale values from other replicas. Local writes are always -immediately visible. With updatePeriod = 0, it always reads fresh synchronized data. - -## Thread Safety - -Operations are atomic when using a single instance. Multiple instances may have -race conditions due to the distributed nature of the system. - -## Example: Cache vs Local Writes +## Example ```go -counter := NewAtomicCounter(db, objectID, fieldOffset, 1*time.Second) - -// Local write - immediately visible -counter.Increment(ctx, 5) // Value: 5 -value, _ := counter.Get(ctx) // Returns 5 immediately - -After sync from other replica (value: 10) -With cache: may still return 5 until cache expires -Without cache: immediately returns 15 (5 + 10) +c := cho.Counter(objectID, fieldOffset) +c.Increment(ctx, 5) // in-memory; no DB write +v, _ := c.Get(ctx) // 5, immediately +// ... background goroutine flushes within CounterSyncPeriod ... ``` diff --git a/counters/atomic_counter.go b/counters/atomic_counter.go index 9e1139e..9499a6c 100644 --- a/counters/atomic_counter.go +++ b/counters/atomic_counter.go @@ -1,17 +1,14 @@ -// Provides AtomicCounter - a high-performance atomic counter implementation -// for distributed systems with CRDT semantics. - +// Package counters provides AtomicCounter, a lock-free CRDT counter (N or Z). +// Get/Increment are lock-free; the manager periodically flushes mine and reloads theirs. +// Increments since last flush are lost on hard crash — deliberate tradeoff for a lock-free hot path. package counters import ( "context" "fmt" - "sync" "sync/atomic" - "time" "github.com/drpcorg/chotki/host" - "github.com/drpcorg/chotki/protocol" "github.com/drpcorg/chotki/rdx" ) @@ -19,166 +16,182 @@ var ErrNotCounter error = fmt.Errorf("not a counter") var ErrCounterNotLoaded error = fmt.Errorf("counter not loaded") var ErrDecrementN error = fmt.Errorf("decrementing natural counter") +// AtomicCounter is the in-memory state for one (rid, offset) counter field; Increment/Get are lock-free. type AtomicCounter struct { - data atomic.Value - db host.Host - rid rdx.ID - offset uint64 - lock sync.RWMutex - expiration time.Time - updatePeriod time.Duration -} - -type atomicNcounter struct { - theirs uint64 - total atomic.Uint64 + data any // *nState | *zState; set once on first successful load + rid rdx.ID + offset uint64 + db host.Host + loaded atomic.Bool + accessed atomic.Bool // set by Get/Increment to request a reload on the next background tick } -type zpart struct { - total int64 - revision int64 +type nState struct { + mine, theirs atomic.Int64 + lastSynced int64 } -type atomicZCounter struct { - theirs int64 - part atomic.Pointer[zpart] +type zState struct { + mine, theirs atomic.Int64 + lastSynced int64 + rev int64 } -// NewAtomicCounter creates a new atomic counter instance. -// -// The counter uses lazy loading with time-based caching. When updatePeriod > 0, -// data is cached to avoid expensive database reads, but may return stale values. -// When updatePeriod = 0, fresh data is always read from the database. -func NewAtomicCounter(db host.Host, rid rdx.ID, offset uint64, updatePeriod time.Duration) *AtomicCounter { - return &AtomicCounter{ - db: db, - rid: rid, - offset: offset, - updatePeriod: updatePeriod, - } +func newAtomicCounter(db host.Host, rid rdx.ID, offset uint64) *AtomicCounter { + return &AtomicCounter{db: db, rid: rid, offset: offset} } -// load retrieves and caches counter data from the database. -// -// Uses double-checked locking: first checks cache without lock, then acquires -// write lock only if cache is expired. Loads TLV data from database and parses -// into internal structures (atomicNcounter for Natural, atomicZCounter for ZCounter). -// This method only affects how frequently we read synchronized data from other replicas. -// Local writes are always immediately visible regardless of cache state. -func (a *AtomicCounter) load() (any, error) { - now := time.Now() - if a.data.Load() != nil && now.Sub(a.expiration) < 0 { - return a.data.Load(), nil - } - - a.lock.RUnlock() - a.lock.Lock() - defer func() { - a.lock.Unlock() - a.lock.RLock() - }() - - if a.data.Load() != nil && now.Sub(a.expiration) < 0 { - return a.data.Load(), nil - } - +// load refreshes theirs from DB; on first call sets counter kind and mine/lastSynced baseline. +// Mutex-free: caller holds the manager mutex. +func (a *AtomicCounter) load() error { rdt, tlv, err := a.db.ObjectFieldTLV(a.rid.ToOff(a.offset)) if err != nil { - return nil, err + return err } - var data any - switch rdt { - case rdx.ZCounter: - total, mine, rev := rdx.Znative3(tlv, a.db.Source()) - part := zpart{total: total, revision: rev} - c := atomicZCounter{ - theirs: total - mine, - part: atomic.Pointer[zpart]{}, + first := !a.loaded.Load() + if first { + switch rdt { + case rdx.Natural: + a.data = &nState{} + case rdx.ZCounter: + a.data = &zState{} + default: + return ErrNotCounter } - c.part.Store(&part) - data = &c - case rdx.Natural: - total, mine := rdx.Nnative2(tlv, a.db.Source()) - c := atomicNcounter{ - theirs: total - mine, - total: atomic.Uint64{}, + } + switch c := a.data.(type) { + case *nState: + sum, mineDB := rdx.Nnative2(tlv, a.db.Source()) + c.theirs.Store(int64(sum) - int64(mineDB)) + if first { + c.mine.Store(int64(mineDB)) + c.lastSynced = int64(mineDB) + } else { + // adopt an external change to our slot (e.g. ORM AddToNField): + // shift mine by the delta and re-baseline, preserving unflushed + // increments so value() stays fresh and nothing is lost to the merge. + if extDelta := int64(mineDB) - c.lastSynced; extDelta != 0 { + c.mine.Add(extDelta) + } + c.lastSynced = int64(mineDB) + } + case *zState: + sum, mineDB, rev := rdx.Znative3(tlv, a.db.Source()) + c.theirs.Store(sum - mineDB) + if first { + c.mine.Store(mineDB) + c.lastSynced = mineDB + c.rev = rev + } else { + // as nState above; also never let our revision fall behind the DB. + if extDelta := mineDB - c.lastSynced; extDelta != 0 { + c.mine.Add(extDelta) + } + c.lastSynced = mineDB + c.rev = max(c.rev, rev) } - c.total.Add(total) - data = &c default: - return nil, ErrNotCounter + return ErrNotCounter } - a.data.Store(data) - a.expiration = now.Add(a.updatePeriod) - return data, nil + a.loaded.Store(true) // publish baseline before any lock-free op + return nil } -// Get retrieves the current value of the counter. +// pendingFlush returns the edit op for a counter with unflushed increments plus +// an onCommit run iff the batch commits (changed=false: nothing to flush). +// Caller holds the manager mutex. // -// Acquires read lock, loads data (cached or from DB), and returns the total value. -// For Natural counters returns sum of all replica contributions, for ZCounter returns current total. -func (a *AtomicCounter) Get(ctx context.Context) (int64, error) { - a.lock.RLock() - defer a.lock.RUnlock() - data, err := a.load() - if err != nil { - return 0, err +// The flush is a read-modify-write: it commits slot + delta (not the cached +// mine), for Z stamps a revision above any present, and onCommit folds the +// observed external change into mine — so a concurrent writer isn't clobbered. +func (a *AtomicCounter) pendingFlush() (changed bool, rdt byte, op []byte, onCommit func()) { + switch c := a.data.(type) { + case *nState: + m := c.mine.Load() + delta := m - c.lastSynced + if delta == 0 { + return false, 0, nil, nil + } + // N is grow-only, so newSlot >= mineDB and the merge always accepts it; + // no revision needed (unlike Z). + _, tlv, err := a.db.ObjectFieldTLV(a.rid.ToOff(a.offset)) + if err != nil { + // log so the skip isn't mistaken for a flush; delta is kept + a.db.Logger().Warn("counter flush: cannot read slot, retrying next cycle", + "rid", a.rid.String(), "offset", a.offset, "err", err) + return false, 0, nil, nil + } + _, mineDB := rdx.Nnative2(tlv, a.db.Source()) + newSlot := int64(mineDB) + delta + extDelta := int64(mineDB) - c.lastSynced // net effect of any other writer to our slot + return true, rdx.Natural, rdx.Ntlvt(uint64(newSlot), a.db.Source()), func() { + if extDelta != 0 { + c.mine.Add(extDelta) // fold the external change into our running total + } + c.lastSynced = newSlot + } + case *zState: + m := c.mine.Load() + delta := m - c.lastSynced + if delta == 0 { + return false, 0, nil, nil + } + _, tlv, err := a.db.ObjectFieldTLV(a.rid.ToOff(a.offset)) + if err != nil { + // log so the skip isn't mistaken for a flush; delta is kept + a.db.Logger().Warn("counter flush: cannot read slot, retrying next cycle", + "rid", a.rid.String(), "offset", a.offset, "err", err) + return false, 0, nil, nil + } + _, mineDB, dbRev := rdx.Znative3(tlv, a.db.Source()) + newRev := max(c.rev, dbRev) + 1 + newSlot := mineDB + delta + extDelta := mineDB - c.lastSynced // net effect of any other writer to our slot + return true, rdx.ZCounter, rdx.Ztlvt(newSlot, a.db.Source(), newRev), func() { + c.rev = newRev + if extDelta != 0 { + c.mine.Add(extDelta) // fold the external change into our running total + } + c.lastSynced = newSlot + } } - switch c := data.(type) { - case *atomicNcounter: - return int64(c.total.Load()), nil - case *atomicZCounter: - return c.part.Load().total, nil - default: + return false, 0, nil, nil +} + +// value returns mine+theirs (lock-free). Assumes the counter is loaded. +func (a *AtomicCounter) value() int64 { + switch c := a.data.(type) { + case *nState: + return c.mine.Load() + c.theirs.Load() + case *zState: + return c.mine.Load() + c.theirs.Load() + } + return 0 +} + +// Get returns mine + last-known others'. Lock-free and DB-free. +func (a *AtomicCounter) Get(ctx context.Context) (int64, error) { + a.accessed.Store(true) + if !a.loaded.Load() { return 0, ErrCounterNotLoaded } + return a.value(), nil } -// Increment atomically increments the counter by the specified value. -// -// Loads current data, performs atomic update using Go primitives (atomic.Uint64 for Natural, -// CompareAndSwap for ZCounter), generates TLV data, and commits to database with CRDT semantics. -// Natural counters only allow positive increments, ZCounter supports both positive and negative. +// Increment adds val to mine (Natural rejects val < 0); flushed by the manager on the next cycle. Lock-free and DB-free. func (a *AtomicCounter) Increment(ctx context.Context, val int64) (int64, error) { - a.lock.RLock() - defer a.lock.RUnlock() - data, err := a.load() - if err != nil { - return 0, err + a.accessed.Store(true) + if !a.loaded.Load() { + return 0, ErrCounterNotLoaded } - var dtlv []byte - var result int64 - var rdt byte - switch c := data.(type) { - case *atomicNcounter: + switch c := a.data.(type) { + case *nState: if val < 0 { return 0, ErrDecrementN } - nw := c.total.Add(uint64(val)) - dtlv = rdx.Ntlvt(nw-c.theirs, a.db.Source()) - result = int64(nw) - rdt = rdx.Natural - case *atomicZCounter: - for { - current := c.part.Load() - nw := zpart{ - total: current.total + val, - revision: current.revision + 1, - } - ok := c.part.CompareAndSwap(current, &nw) - if ok { - dtlv = rdx.Ztlvt(nw.total-c.theirs, a.db.Source(), nw.revision) - result = nw.total - rdt = rdx.ZCounter - break - } - } - default: - return 0, ErrCounterNotLoaded + return c.mine.Add(val) + c.theirs.Load(), nil + case *zState: + return c.mine.Add(val) + c.theirs.Load(), nil } - changes := make(protocol.Records, 0) - changes = append(changes, protocol.Record('F', rdx.ZipUint64(uint64(a.offset)))) - changes = append(changes, protocol.Record(rdt, dtlv)) - a.db.CommitPacket(ctx, 'E', a.rid.ZeroOff(), changes) - return result, nil + return 0, ErrCounterNotLoaded } diff --git a/counters/atomic_counter_test.go b/counters/atomic_counter_test.go index cb0e574..2247fae 100644 --- a/counters/atomic_counter_test.go +++ b/counters/atomic_counter_test.go @@ -4,6 +4,8 @@ import ( "context" "fmt" "os" + "sync" + "sync/atomic" "testing" "time" @@ -11,112 +13,627 @@ import ( "github.com/drpcorg/chotki" "github.com/drpcorg/chotki/classes" "github.com/drpcorg/chotki/counters" + "github.com/drpcorg/chotki/host" "github.com/drpcorg/chotki/protocol" "github.com/drpcorg/chotki/rdx" testutils "github.com/drpcorg/chotki/test_utils" "github.com/stretchr/testify/assert" ) -func TestAtomicCounter(t *testing.T) { +// openReplica opens a fresh replica; a long period prevents background cycles so tests drive them manually. +func openReplica(t *testing.T, src uint64, period time.Duration) *chotki.Chotki { + t.Helper() dir, err := os.MkdirTemp("", "*") assert.NoError(t, err) - - a, err := chotki.Open(dir, chotki.Options{ - Src: 0x1a, - Name: "test replica", - Options: pebble.Options{ErrorIfExists: true}, + cho, err := chotki.Open(dir, chotki.Options{ + Src: src, + Name: "replica", + Options: pebble.Options{ErrorIfExists: true}, + CounterSyncPeriod: period, }) assert.NoError(t, err) + t.Cleanup(func() { _ = cho.Close() }) + return cho +} - cid, err := a.NewClass(context.Background(), rdx.ID0, classes.Field{Name: "test", RdxType: rdx.Natural}) +// newCounterObject creates a class with an N field (offset 1) and a Z field (offset 2), then returns the object id. +func newCounterObject(t *testing.T, cho *chotki.Chotki) rdx.ID { + t.Helper() + cid, err := cho.NewClass(context.Background(), rdx.ID0, + classes.Field{Name: "n", RdxType: rdx.Natural}, + classes.Field{Name: "z", RdxType: rdx.ZCounter}, + ) + assert.NoError(t, err) + rid, err := cho.NewObjectTLV(context.Background(), cid, + protocol.Records{ + protocol.Record('N', rdx.Ntlv(0)), + protocol.Record('Z', rdx.Ztlv(0)), + }) assert.NoError(t, err) + return rid +} - rid, err := a.NewObjectTLV(context.Background(), cid, protocol.Records{protocol.Record('N', rdx.Ntlv(0))}) +// persistedN reads the Natural field's total directly from the DB, bypassing in-memory state. +func persistedN(t *testing.T, cho *chotki.Chotki, rid rdx.ID, offset uint64) int64 { + t.Helper() + rdt, tlv, err := cho.ObjectFieldTLV(rid.ToOff(offset)) assert.NoError(t, err) + assert.EqualValues(t, rdx.Natural, rdt) + sum, _ := rdx.Nnative2(tlv, cho.Source()) + return int64(sum) +} - counterA := counters.NewAtomicCounter(a, rid, 1, 0) - counterB := counters.NewAtomicCounter(a, rid, 1, 0) +// persistedZ is persistedN for a ZCounter field. +func persistedZ(t *testing.T, cho *chotki.Chotki, rid rdx.ID, offset uint64) int64 { + t.Helper() + rdt, tlv, err := cho.ObjectFieldTLV(rid.ToOff(offset)) + assert.NoError(t, err) + assert.EqualValues(t, rdx.ZCounter, rdt) + sum, _, _ := rdx.Znative3(tlv, cho.Source()) + return sum +} + +func TestAtomicCounter(t *testing.T) { + ctx := context.Background() + a := openReplica(t, 0x1a, time.Hour) + rid := newCounterObject(t, a) - res, err := counterA.Increment(context.Background(), 1) + // Two handles for the same field share state. + ca := a.Counter(rid, 1) + cb := a.Counter(rid, 1) + + res, err := ca.Increment(ctx, 1) assert.NoError(t, err) assert.EqualValues(t, 1, res) - res, err = counterB.Increment(context.Background(), 1) + res, err = cb.Increment(ctx, 1) assert.NoError(t, err) assert.EqualValues(t, 2, res) - res, err = counterA.Increment(context.Background(), 1) + res, err = ca.Increment(ctx, 1) assert.NoError(t, err) assert.EqualValues(t, 3, res) } -func TestAtomicCounterWithPeriodicUpdate(t *testing.T) { - dira, err := os.MkdirTemp("", "*") +func TestBatchedNoLossNatural(t *testing.T) { + ctx := context.Background() + a := openReplica(t, 0x1a, time.Hour) + rid := newCounterObject(t, a) + + c := a.Counter(rid, 1) // batched, lock-free + for i := 0; i < 5; i++ { + _, err := c.Increment(ctx, 1) + assert.NoError(t, err) + } + + // Local increments are visible before flush; DB is 0 until a cycle runs. + got, err := c.Get(ctx) assert.NoError(t, err) + assert.EqualValues(t, 5, got) + assert.EqualValues(t, 0, persistedN(t, a, rid, 1)) - a, err := chotki.Open(dira, chotki.Options{ - Src: 0x1a, - Name: "test replica", - Options: pebble.Options{ErrorIfExists: true}, - }) + // One cycle flushes; fresh DB read confirms persistence. + a.SyncCounters(ctx) + assert.EqualValues(t, 5, persistedN(t, a, rid, 1)) + + got, err = c.Get(ctx) assert.NoError(t, err) + assert.EqualValues(t, 5, got) +} - dirb, err := os.MkdirTemp("", "*") +func TestBatchedZCounterTwoWay(t *testing.T) { + ctx := context.Background() + a := openReplica(t, 0x1a, time.Hour) + rid := newCounterObject(t, a) + + cz := a.Counter(rid, 2) // Z field + _, err := cz.Increment(ctx, 10) + assert.NoError(t, err) + _, err = cz.Increment(ctx, -3) assert.NoError(t, err) - b, err := chotki.Open(dirb, chotki.Options{ - Src: 0x1b, - Name: "test replica2", - Options: pebble.Options{ErrorIfExists: true}, - }) + got, err := cz.Get(ctx) assert.NoError(t, err) + assert.EqualValues(t, 7, got) + assert.EqualValues(t, 0, persistedZ(t, a, rid, 2)) // unflushed - cid, err := a.NewClass( - context.Background(), rdx.ID0, - classes.Field{Name: "test", RdxType: rdx.Natural}, - classes.Field{Name: "test2", RdxType: rdx.ZCounter}, - ) + a.SyncCounters(ctx) + assert.EqualValues(t, 7, persistedZ(t, a, rid, 2)) + + got, err = cz.Get(ctx) assert.NoError(t, err) + assert.EqualValues(t, 7, got) +} - rid, err := a.NewObjectTLV( - context.Background(), cid, - protocol.Records{ - protocol.Record('N', rdx.Ntlv(0)), - protocol.Record('Z', rdx.Ztlv(0)), - }, - ) +// A flush must apply the manager's accumulated increments as a DELTA on top of the current +// DB value of its own src slot, stamped above any revision already there -- so a second +// writer to the same (src, field) slot (as the keymanager ORM Zdelta "set" path does) is +// preserved additively and the manager's own increment is never silently dropped by merge. +func TestZCounterFlushAppliesDeltaOverExternalWrite(t *testing.T) { + ctx := context.Background() + a := openReplica(t, 0x1a, time.Hour) + rid := newCounterObject(t, a) + + c := a.Counter(rid, 2) // Z field + _, err := c.Increment(ctx, 10) assert.NoError(t, err) + a.SyncCounters(ctx) // DB slot = 10 at rev 1 + assert.EqualValues(t, 10, persistedZ(t, a, rid, 2)) - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() + // Second writer to the SAME src slot, bypassing the manager: raise the slot to 100 with + // a fresh revision (exactly what rdx.Zdelta from an ORM object save produces). + _, oldTlv, err := a.ObjectFieldTLV(rid.ToOff(2)) + assert.NoError(t, err) + _, err = a.EditFieldTLV(ctx, rid.ToOff(2), + protocol.Record(rdx.ZCounter, rdx.Zdelta(oldTlv, 100, a.Clock()))) + assert.NoError(t, err) + assert.EqualValues(t, 100, persistedZ(t, a, rid, 2)) - for i := 1; i <= 2; i++ { + // Manager accumulates +5 more, then flushes. The external 100 must survive and the +5 + // must be added on top -> 105. Buggy code writes absolute mine (15) at a stale rev, so + // the merge either drops the increment or clobbers the external write. + _, err = c.Increment(ctx, 5) + assert.NoError(t, err) + a.SyncCounters(ctx) - counterA := counters.NewAtomicCounter(a, rid, uint64(i), 100*time.Millisecond) - counterB := counters.NewAtomicCounter(b, rid, uint64(i), 0) + assert.EqualValues(t, 105, persistedZ(t, a, rid, 2)) + got, err := c.Get(ctx) + assert.NoError(t, err) + assert.EqualValues(t, 105, got) +} - // first increment - res, err := counterA.Increment(ctx, 1) - assert.NoError(t, err) - assert.EqualValues(t, 1, res, fmt.Sprintf("iteration %d", i)) - testutils.SyncData(a, b) +// After a second writer changes the slot, a reload with nothing to flush must still adopt the +// external value so Get() reflects it -- not only after the next local increment rebases mine. +func TestZCounterReloadAdoptsExternalWrite(t *testing.T) { + ctx := context.Background() + a := openReplica(t, 0x1a, time.Hour) + rid := newCounterObject(t, a) - // increment from another replica - res, err = counterB.Increment(ctx, 1) - assert.NoError(t, err) - assert.EqualValues(t, 2, res, fmt.Sprintf("iteration %d", i)) + c := a.Counter(rid, 2) + _, err := c.Increment(ctx, 10) + assert.NoError(t, err) + a.SyncCounters(ctx) + assert.EqualValues(t, 10, persistedZ(t, a, rid, 2)) + + // External writer raises the slot to 100; the manager makes no local increment. + _, oldTlv, err := a.ObjectFieldTLV(rid.ToOff(2)) + assert.NoError(t, err) + _, err = a.EditFieldTLV(ctx, rid.ToOff(2), + protocol.Record(rdx.ZCounter, rdx.Zdelta(oldTlv, 100, a.Clock()))) + assert.NoError(t, err) + + a.SyncCounters(ctx) // reload only; there is nothing local to flush + + got, err := c.Get(ctx) + assert.NoError(t, err) + assert.EqualValues(t, 100, got) +} + +func TestNaturalRejectsDecrement(t *testing.T) { + ctx := context.Background() + a := openReplica(t, 0x1a, time.Hour) + rid := newCounterObject(t, a) + + _, err := a.Counter(rid, 1).Increment(ctx, -1) + assert.ErrorIs(t, err, counters.ErrDecrementN) +} + +func TestNotLoadedThenRetry(t *testing.T) { + ctx := context.Background() + a := openReplica(t, 0x1a, time.Hour) + b := openReplica(t, 0x1b, time.Hour) + rid := newCounterObject(t, a) + + // b lacks the object -> initial load fails. + cb := b.Counter(rid, 1) + _, err := cb.Get(ctx) + assert.ErrorIs(t, err, counters.ErrCounterNotLoaded) + + // Propagate a value to b; b's next cycle retries the load. + ca := a.Counter(rid, 1) + _, err = ca.Increment(ctx, 4) + assert.NoError(t, err) + a.SyncCounters(ctx) + testutils.SyncData(a, b) // returns io.EOF on normal completion + b.SyncCounters(ctx) + + got, err := cb.Get(ctx) + assert.NoError(t, err) + assert.EqualValues(t, 4, got) +} + +// A counter whose object wasn't local at first touch must load on the next Counter() +// lookup once the object is readable, WITHOUT waiting for a background cycle. Regression: +// the manager loaded once at handle creation and cached the unloaded handle, so a +// just-synced counter stayed ErrCounterNotLoaded until the next tick. Callers (e.g. dproxy) +// re-resolve via Counter() on every op, so Counter() must return a loaded handle. +func TestLazyLoadOnAccessAfterSync(t *testing.T) { + ctx := context.Background() + a := openReplica(t, 0x1a, time.Hour) + b := openReplica(t, 0x1b, time.Hour) + rid := newCounterObject(t, a) + + // b touches the counter before it has the object -> handle cached unloaded. + _, err := b.Counter(rid, 1).Get(ctx) + assert.ErrorIs(t, err, counters.ErrCounterNotLoaded) + + // a contributes 4; replicate the object+value to b. Crucially, do NOT run b's + // counter cycle -- recovery must come from Counter() reloading on the next lookup. + ca := a.Counter(rid, 1) + _, err = ca.Increment(ctx, 4) + assert.NoError(t, err) + a.SyncCounters(ctx) + testutils.SyncData(a, b) + + // A fresh Counter() lookup (as callers do per op) must return a loaded handle. + got, err := b.Counter(rid, 1).Get(ctx) + assert.NoError(t, err) + assert.EqualValues(t, 4, got) + + // Increment via a fresh lookup also works and adds to the loaded value. + got, err = b.Counter(rid, 1).Increment(ctx, 1) + assert.NoError(t, err) + assert.EqualValues(t, 5, got) +} + +func TestConcurrentIncrementsNoLoss(t *testing.T) { + ctx := context.Background() + a := openReplica(t, 0x1a, time.Hour) + rid := newCounterObject(t, a) + + c := a.Counter(rid, 1) // loaded at creation (object exists) + + const goroutines, perG = 8, 100 + var wg sync.WaitGroup + for g := 0; g < goroutines; g++ { + wg.Add(1) + go func() { + defer wg.Done() + for i := 0; i < perG; i++ { + if _, err := c.Increment(ctx, 1); err != nil { + t.Errorf("increment: %v", err) + } + } + }() + } + wg.Wait() + + got, err := c.Get(ctx) + assert.NoError(t, err) + assert.EqualValues(t, goroutines*perG, got) + + a.SyncCounters(ctx) + assert.EqualValues(t, goroutines*perG, persistedN(t, a, rid, 1)) +} + +func TestEventualConsistencyTwoReplicas(t *testing.T) { + ctx := context.Background() + a := openReplica(t, 0x1a, time.Hour) + b := openReplica(t, 0x1b, time.Hour) + rid := newCounterObject(t, a) + testutils.SyncData(a, b) // b gets the class + object + + ca := a.Counter(rid, 1) + cb := b.Counter(rid, 1) + + _, err := ca.Increment(ctx, 3) + assert.NoError(t, err) + _, err = cb.Increment(ctx, 5) + assert.NoError(t, err) + + // Before sync, each replica sees only its own contribution. + gotA0, err := ca.Get(ctx) + assert.NoError(t, err) + assert.EqualValues(t, 3, gotA0) + gotB0, err := cb.Get(ctx) + assert.NoError(t, err) + assert.EqualValues(t, 5, gotB0) + + // Flush, exchange, reload both sides. + a.SyncCounters(ctx) + b.SyncCounters(ctx) + testutils.SyncData(a, b) + a.SyncCounters(ctx) + b.SyncCounters(ctx) + + gotA, err := ca.Get(ctx) + assert.NoError(t, err) + gotB, err := cb.Get(ctx) + assert.NoError(t, err) + assert.EqualValues(t, 8, gotA) + assert.EqualValues(t, 8, gotB) +} + +// Headline: no missed events — every increment on A must appear on B exactly. +func TestNoMissedEventsOverManyCycles(t *testing.T) { + ctx := context.Background() + a := openReplica(t, 0x1a, time.Hour) + b := openReplica(t, 0x1b, time.Hour) + rid := newCounterObject(t, a) + testutils.SyncData(a, b) + + ca := a.Counter(rid, 1) + cb := b.Counter(rid, 1) + + total := int64(0) + for round := 1; round <= 10; round++ { + for i := 0; i < round; i++ { // varying burst sizes + _, err := ca.Increment(ctx, 1) + assert.NoError(t, err) + total++ + } + a.SyncCounters(ctx) testutils.SyncData(a, b) + b.SyncCounters(ctx) + } + + got, err := cb.Get(ctx) + assert.NoError(t, err) + assert.EqualValues(t, total, got, "b must see every a increment") +} + +func TestManyCountersOneCycle(t *testing.T) { + ctx := context.Background() + a := openReplica(t, 0x1a, time.Hour) - // this increment does not account data from other replica because current value is cached - res, err = counterA.Increment(ctx, 1) + cid, err := a.NewClass(ctx, rdx.ID0, classes.Field{Name: "n", RdxType: rdx.Natural}) + assert.NoError(t, err) + + const n = 50 + rids := make([]rdx.ID, n) + for i := 0; i < n; i++ { + rid, err := a.NewObjectTLV(ctx, cid, protocol.Records{protocol.Record('N', rdx.Ntlv(0))}) + assert.NoError(t, err) + rids[i] = rid + _, err = a.Counter(rid, 1).Increment(ctx, int64(i+1)) assert.NoError(t, err) - assert.EqualValues(t, 2, res, fmt.Sprintf("iteration %d", i)) + } + + a.SyncCounters(ctx) // one cycle flushes all 50 + + for i := 0; i < n; i++ { + assert.EqualValues(t, i+1, persistedN(t, a, rids[i], 1)) + } +} - time.Sleep(100 * time.Millisecond) +func TestGracefulShutdownFlushes(t *testing.T) { + ctx := context.Background() + dir, err := os.MkdirTemp("", "*") + assert.NoError(t, err) + + a, err := chotki.Open(dir, chotki.Options{ + Src: 0x1a, Name: "replica", + Options: pebble.Options{ErrorIfExists: true}, + CounterSyncPeriod: time.Hour, // no background cycle; flush happens on Close + }) + assert.NoError(t, err) + rid := newCounterObject(t, a) - // after wait we increment, and we get actual value - res, err = counterA.Increment(ctx, 1) + c := a.Counter(rid, 1) + _, err = c.Increment(ctx, 5) // not yet flushed + assert.NoError(t, err) + assert.NoError(t, a.Close()) // shutdown must flush the 5 + + // Reopen the same directory (it now exists). + a2, err := chotki.Open(dir, chotki.Options{ + Src: 0x1a, Name: "replica", + Options: pebble.Options{ErrorIfExists: false}, + CounterSyncPeriod: time.Hour, + }) + assert.NoError(t, err) + defer a2.Close() + + assert.EqualValues(t, 5, persistedN(t, a2, rid, 1)) + got, err := a2.Counter(rid, 1).Get(ctx) // fresh handle loaded from DB + assert.NoError(t, err) + assert.EqualValues(t, 5, got) +} + +// Z counters carry a per-source revision; it must survive reopen or a post-reopen flush emits a stale rev and is silently dropped. +func TestZCounterRevisionAcrossReopen(t *testing.T) { + ctx := context.Background() + dir, err := os.MkdirTemp("", "*") + assert.NoError(t, err) + + a, err := chotki.Open(dir, chotki.Options{ + Src: 0x1a, Name: "replica", + Options: pebble.Options{ErrorIfExists: true}, + CounterSyncPeriod: time.Hour, + }) + assert.NoError(t, err) + rid := newCounterObject(t, a) + _, err = a.Counter(rid, 2).Increment(ctx, 7) + assert.NoError(t, err) + a.SyncCounters(ctx) // persists Z at rev 1 + assert.NoError(t, a.Close()) + + a2, err := chotki.Open(dir, chotki.Options{ + Src: 0x1a, Name: "replica", + Options: pebble.Options{ErrorIfExists: false}, + CounterSyncPeriod: time.Hour, + }) + assert.NoError(t, err) + got, err := a2.Counter(rid, 2).Increment(ctx, 3) // must use rev 2 > persisted rev 1 + assert.NoError(t, err) + assert.EqualValues(t, 10, got) + a2.SyncCounters(ctx) + assert.NoError(t, a2.Close()) + + a3, err := chotki.Open(dir, chotki.Options{ + Src: 0x1a, Name: "replica", + Options: pebble.Options{ErrorIfExists: false}, + CounterSyncPeriod: time.Hour, + }) + assert.NoError(t, err) + defer a3.Close() + assert.EqualValues(t, 10, persistedZ(t, a3, rid, 2)) // both flush generations persisted + got, err = a3.Counter(rid, 2).Get(ctx) + assert.NoError(t, err) + assert.EqualValues(t, 10, got) +} + +// Concurrent Counter() calls for the same field must all share one state and be race-free. +func TestConcurrentCounterCreation(t *testing.T) { + ctx := context.Background() + a := openReplica(t, 0x1a, time.Hour) + rid := newCounterObject(t, a) + + const n = 20 + handles := make([]*counters.AtomicCounter, n) + var wg sync.WaitGroup + for i := 0; i < n; i++ { + wg.Add(1) + go func(i int) { + defer wg.Done() + handles[i] = a.Counter(rid, 1) + }(i) + } + wg.Wait() + + // All handles share state: n increments of 1 must total n. + for _, h := range handles { + _, err := h.Increment(ctx, 1) assert.NoError(t, err) - assert.EqualValues(t, 4, res, fmt.Sprintf("iteration %d", i)) } + got, err := handles[0].Get(ctx) + assert.NoError(t, err) + assert.EqualValues(t, n, got) + + a.SyncCounters(ctx) + assert.EqualValues(t, n, persistedN(t, a, rid, 1)) +} + +// batchFaultHost wraps a host and fails the whole CommitBatch when armed. +type batchFaultHost struct { + host.Host + fail atomic.Bool +} + +func (f *batchFaultHost) CommitBatch(ctx context.Context, edits []host.Edit) error { + if f.fail.Load() { + return fmt.Errorf("injected batch flush failure") + } + return f.Host.CommitBatch(ctx, edits) +} + +// Batch commit failure is all-or-nothing: nothing persisted, nothing marked; retained increments flush on the next cycle. +func TestBatchFlushFailureRetried(t *testing.T) { + ctx := context.Background() + a := openReplica(t, 0x1a, time.Hour) + + cid, err := a.NewClass(ctx, rdx.ID0, classes.Field{Name: "n", RdxType: rdx.Natural}) + assert.NoError(t, err) + rid1, err := a.NewObjectTLV(ctx, cid, protocol.Records{protocol.Record('N', rdx.Ntlv(0))}) + assert.NoError(t, err) + rid2, err := a.NewObjectTLV(ctx, cid, protocol.Records{protocol.Record('N', rdx.Ntlv(0))}) + assert.NoError(t, err) + + fh := &batchFaultHost{Host: a} + mgr := counters.NewAtomicCounterManager(fh, 0, nil) + _, err = mgr.Counter(rid1, 1).Increment(ctx, 3) + assert.NoError(t, err) + _, err = mgr.Counter(rid2, 1).Increment(ctx, 7) + assert.NoError(t, err) + + fh.fail.Store(true) + mgr.Cycle(ctx) // batch fails: nothing persisted + assert.EqualValues(t, 0, persistedN(t, a, rid1, 1)) + assert.EqualValues(t, 0, persistedN(t, a, rid2, 1)) + + fh.fail.Store(false) + mgr.Cycle(ctx) // retry flushes retained increments + assert.EqualValues(t, 3, persistedN(t, a, rid1, 1)) + assert.EqualValues(t, 7, persistedN(t, a, rid2, 1)) +} + +// The background ticker (not just manual SyncCounters) must flush counters. +func TestBackgroundTimerFlushes(t *testing.T) { + ctx := context.Background() + a := openReplica(t, 0x1a, 20*time.Millisecond) // real, short period + + rid := newCounterObject(t, a) + _, err := a.Counter(rid, 1).Increment(ctx, 9) // no manual SyncCounters + assert.NoError(t, err) + + assert.Eventually(t, func() bool { + rdt, tlv, err := a.ObjectFieldTLV(rid.ToOff(1)) + if err != nil || rdt != rdx.Natural { + return false + } + sum, _ := rdx.Nnative2(tlv, a.Source()) + return sum == 9 + }, 2*time.Second, 10*time.Millisecond) +} + +// A failed baseline load must keep being retried by the ticker until data arrives, even if never re-accessed. +func TestNotLoadedRetriedByBackgroundTick(t *testing.T) { + ctx := context.Background() + a := openReplica(t, 0x1a, time.Hour) + b := openReplica(t, 0x1b, 20*time.Millisecond) // real, short period + rid := newCounterObject(t, a) + + _, err := a.Counter(rid, 1).Increment(ctx, 6) + assert.NoError(t, err) + a.SyncCounters(ctx) + + cb := b.Counter(rid, 1) // load fails: b lacks the object + _, err = cb.Get(ctx) // arms the accessed-retry signal + assert.ErrorIs(t, err, counters.ErrCounterNotLoaded) + + // Let a tick consume the accessed signal while data is still absent, so recovery + // can only come from retrying unloaded fields — not the stale signal. + time.Sleep(100 * time.Millisecond) + testutils.SyncData(a, b) // b now has the object + + // Wait for ticker alone; no Get during wait (would re-arm retry, masking regressions). + time.Sleep(400 * time.Millisecond) + got, err := cb.Get(ctx) + assert.NoError(t, err) + assert.EqualValues(t, 6, got) +} + +// Increments from many goroutines concurrent with flush cycles; must be race-free and lose no increments. +func TestConcurrentIncrementDuringCycles(t *testing.T) { + ctx := context.Background() + a := openReplica(t, 0x1a, time.Hour) // manual cycles maximize contention + rid := newCounterObject(t, a) + c := a.Counter(rid, 1) + + const goroutines, perG = 8, 200 + stop := make(chan struct{}) + var cyc sync.WaitGroup + cyc.Add(1) + go func() { + defer cyc.Done() + for { + select { + case <-stop: + return + default: + a.SyncCounters(ctx) // flush+reload concurrent with increments + } + } + }() + + var wg sync.WaitGroup + for g := 0; g < goroutines; g++ { + wg.Add(1) + go func() { + defer wg.Done() + for i := 0; i < perG; i++ { + if _, err := c.Increment(ctx, 1); err != nil { + t.Errorf("increment: %v", err) + } + } + }() + } + wg.Wait() + close(stop) + cyc.Wait() + + a.SyncCounters(ctx) // final flush + assert.EqualValues(t, goroutines*perG, persistedN(t, a, rid, 1)) + got, err := c.Get(ctx) + assert.NoError(t, err) + assert.EqualValues(t, goroutines*perG, got) } diff --git a/counters/manager.go b/counters/manager.go new file mode 100644 index 0000000..9ead996 --- /dev/null +++ b/counters/manager.go @@ -0,0 +1,196 @@ +package counters + +import ( + "context" + "errors" + "sync" + "time" + + lru "github.com/hashicorp/golang-lru/v2/expirable" + + "github.com/drpcorg/chotki/host" + "github.com/drpcorg/chotki/protocol" + "github.com/drpcorg/chotki/rdx" + "github.com/drpcorg/chotki/utils" +) + +// negCacheTTL bounds how long a "field is not a counter" result is cached; a var +// so tests can shrink it. +var negCacheTTL = time.Second + +// negCacheSize caps the number of negative entries (LRU eviction). +const negCacheSize = 1024 + +// AtomicCounterManager owns per-field AtomicCounters; a mutex serializes DB I/O while the hot +// path (Get/Increment) never takes it. +type AtomicCounterManager struct { + mu sync.Mutex + period time.Duration + states sync.Map // rdx.ID (rid.ToOff(offset)) -> *AtomicCounter + db host.Host + log utils.Logger + // negCache rate-limits load retries for not-a-counter fields (ErrNotCounter), + // which would otherwise re-hit the DB and log on every call/tick. Entries + // expire so a field that becomes a counter self-heals (not a permanent latch). + negCache *lru.LRU[rdx.ID, struct{}] +} + +func NewAtomicCounterManager(db host.Host, period time.Duration, log utils.Logger) *AtomicCounterManager { + return &AtomicCounterManager{ + db: db, + period: period, + log: log, + negCache: lru.NewLRU[rdx.ID, struct{}](negCacheSize, nil, negCacheTTL), + } +} + +// Counter returns the counter for (rid, offset), creating and loading it on first use; a load that +// failed (object not local yet) is retried on each call and by the background cycle. +func (m *AtomicCounterManager) Counter(rid rdx.ID, offset uint64) *AtomicCounter { + key := rid.ToOff(offset) + existing, ok := m.states.Load(key) + if !ok { + // lost races fall through to the winner's counter + existing, _ = m.states.LoadOrStore(key, newAtomicCounter(m.db, rid, offset)) + } + c := existing.(*AtomicCounter) + m.ensureLoaded(c) // first load, or retry of a previously-failed one; no-op once loaded + return c +} + +// ensureLoaded retries the load for a cached counter whose object wasn't local at first touch; +// no-op once loaded. Serialized with flush/reload via m.mu; the hot path never reaches the lock. +func (m *AtomicCounterManager) ensureLoaded(c *AtomicCounter) { + if c.loaded.Load() { + return + } + m.mu.Lock() + defer m.mu.Unlock() + if c.loaded.Load() { + return + } + m.loadLocked(c) +} + +// loadLocked runs c.load() unless the field was recently found to be not a +// counter (negCache). On success it clears accessed; on a transient failure it +// leaves accessed set so the next cycle retries. Caller holds m.mu. +func (m *AtomicCounterManager) loadLocked(c *AtomicCounter) { + key := c.rid.ToOff(c.offset) + if _, bad := m.negCache.Get(key); bad { + return // recently not-a-counter; skip the DB hit, expires soon + } + if err := c.load(); err != nil { + if errors.Is(err, ErrNotCounter) { + m.negCache.Add(key, struct{}{}) + } + if m.log != nil { + m.log.Warn("counter load failed", "rid", c.rid.String(), "offset", c.offset, "err", err) + } + return + } + c.accessed.Store(false) // only after a successful load +} + +// Cycle forces a flush + full reload of all counters; used by SyncCounters and tests. +func (m *AtomicCounterManager) Cycle(ctx context.Context) { + m.cycle(ctx, true) +} + +// cycle flushes dirty counters, then reloads all (force) or only touched/unloaded ones (background tick uses force=false). +func (m *AtomicCounterManager) cycle(ctx context.Context, force bool) { + m.mu.Lock() + defer m.mu.Unlock() + + // snapshot states once, then drive both flush and reload from it. (No + // eviction — states grows with the number of distinct counters touched.) + snapshot := m.snapshotLocked() + + m.flushLocked(ctx, snapshot) + + for _, c := range snapshot { + // accessed is read, not swapped: loadLocked clears it only on a + // successful load, so a transient failure keeps the reload pending. + if c.accessed.Load() || force || !c.loaded.Load() { + m.loadLocked(c) + } + } +} + +// snapshotLocked collects the current counters into a slice. Caller holds m.mu. +func (m *AtomicCounterManager) snapshotLocked() []*AtomicCounter { + snapshot := make([]*AtomicCounter, 0) + m.states.Range(func(_, v any) bool { + snapshot = append(snapshot, v.(*AtomicCounter)) + return true + }) + return snapshot +} + +// maxFlushBatch caps edits per CommitBatch; a var so tests can shrink it. +var maxFlushBatch = 1024 + +// flushLocked commits changed counters in maxFlushBatch chunks; a failed chunk +// is retried next cycle. Caller holds m.mu. It runs inside the sequential-write +// bracket (spanning all chunks, since every op is read before the first commit) +// so a concurrent RMW writer of the same slot can't collide and lose a write. +func (m *AtomicCounterManager) flushLocked(ctx context.Context, snapshot []*AtomicCounter) { + m.db.StartSequentialWrite() + defer m.db.EndSequentialWrite() + var edits []host.Edit + var commits []func() + for _, c := range snapshot { + changed, rdt, op, onCommit := c.pendingFlush() + if !changed { + continue + } + edits = append(edits, host.Edit{ + Ref: c.rid.ZeroOff(), + Body: protocol.Records{ + protocol.Record('F', rdx.ZipUint64(c.offset)), + protocol.Record(rdt, op), + }, + }) + commits = append(commits, onCommit) + } + // Each chunk is all-or-nothing; a failed chunk is retried next cycle. + for start := 0; start < len(edits); start += maxFlushBatch { + end := min(start+maxFlushBatch, len(edits)) + if err := m.db.CommitBatch(ctx, edits[start:end]); err != nil { + if m.log != nil { + m.log.Warn("counter batch flush failed", "edits", end-start, "err", err) + } + continue // this chunk is retried next cycle + } + for _, onCommit := range commits[start:end] { + onCommit() + } + } +} + +// Run is the background goroutine; ticks cycle every period and flushes on shutdown. +func (m *AtomicCounterManager) Run(ctx context.Context) { + if m.period <= 0 { + <-ctx.Done() + m.flushOnShutdown() + return + } + t := time.NewTicker(m.period) + defer t.Stop() + for { + select { + case <-ctx.Done(): + m.flushOnShutdown() + return + case <-t.C: + m.cycle(ctx, false) + } + } +} + +func (m *AtomicCounterManager) flushOnShutdown() { + m.mu.Lock() + defer m.mu.Unlock() + // CommitBatch ignores cancellation internally, so Background is safe. + m.flushLocked(context.Background(), m.snapshotLocked()) +} diff --git a/counters/manager_internal_test.go b/counters/manager_internal_test.go new file mode 100644 index 0000000..e8488df --- /dev/null +++ b/counters/manager_internal_test.go @@ -0,0 +1,136 @@ +package counters + +import ( + "context" + "errors" + "sync" + "testing" + + "github.com/drpcorg/chotki/host" + "github.com/drpcorg/chotki/rdx" + "github.com/stretchr/testify/assert" +) + +// mockHost implements the flush/load path of host.Host and records CommitBatch call sizes. +type mockHost struct { + host.Host + mu sync.Mutex + batches []int +} + +func (h *mockHost) Source() uint64 { return 0x1a } + +func (h *mockHost) ObjectFieldTLV(fid rdx.ID) (byte, []byte, error) { + return rdx.Natural, rdx.Ntlv(0), nil +} + +func (h *mockHost) CommitBatch(ctx context.Context, edits []host.Edit) error { + h.mu.Lock() + h.batches = append(h.batches, len(edits)) + h.mu.Unlock() + return nil +} + +func (h *mockHost) StartSequentialWrite() {} +func (h *mockHost) EndSequentialWrite() {} + +// Cycle splits >maxFlushBatch dirty counters into chunks of at most maxFlushBatch. +func TestFlushChunking(t *testing.T) { + old := maxFlushBatch + maxFlushBatch = 2 + defer func() { maxFlushBatch = old }() + + h := &mockHost{} + m := NewAtomicCounterManager(h, 0, nil) + for i := 0; i < 5; i++ { + rid := rdx.IDFromSrcSeqOff(0x1a, uint64(i+1), 0) + _, err := m.Counter(rid, 1).Increment(context.Background(), 1) + assert.NoError(t, err) + } + + m.Cycle(context.Background()) + + // 5 changed counters, chunked by 2 -> commits of sizes [2, 2, 1] + assert.Equal(t, []int{2, 2, 1}, h.batches) +} + +// typeMockHost reports a non-counter field type and counts load attempts. +type typeMockHost struct { + host.Host + mu sync.Mutex + loads int +} + +func (h *typeMockHost) Source() uint64 { return 0x1a } +func (h *typeMockHost) ObjectFieldTLV(fid rdx.ID) (byte, []byte, error) { + h.mu.Lock() + h.loads++ + h.mu.Unlock() + return rdx.String, nil, nil // not a counter -> load() returns ErrNotCounter +} +func (h *typeMockHost) CommitBatch(ctx context.Context, edits []host.Edit) error { return nil } +func (h *typeMockHost) StartSequentialWrite() {} +func (h *typeMockHost) EndSequentialWrite() {} +func (h *typeMockHost) loadCount() int { + h.mu.Lock() + defer h.mu.Unlock() + return h.loads +} + +// A field that is not a counter must be loaded once and then negatively cached, +// so repeated Counter() calls and background ticks don't re-hit the DB every +// time. The cache is time-bounded (negCacheTTL), not a permanent +// latch, so a field that later becomes a counter self-heals. +func TestNotACounterIsNegativelyCached(t *testing.T) { + h := &typeMockHost{} + m := NewAtomicCounterManager(h, 0, nil) + rid := rdx.IDFromSrcSeqOff(0x1a, 1, 0) + for i := 0; i < 5; i++ { + _ = m.Counter(rid, 1) + } + m.Cycle(context.Background()) + assert.Equal(t, 1, h.loadCount(), + "a not-a-counter field must be loaded once, then served from the negative cache") +} + +var errNotSynced = errors.New("not synced yet") + +// flakyHost fails the first failCount load attempts (as if the object isn't +// synced yet — NOT a wrong-type error, so it must not be negatively cached), +// then returns a valid counter. +type flakyHost struct { + host.Host + mu sync.Mutex + calls int + failCount int +} + +func (h *flakyHost) Source() uint64 { return 0x1a } +func (h *flakyHost) ObjectFieldTLV(fid rdx.ID) (byte, []byte, error) { + h.mu.Lock() + defer h.mu.Unlock() + h.calls++ + if h.calls <= h.failCount { + return 0, nil, errNotSynced + } + return rdx.Natural, rdx.Ntlv(0), nil +} +func (h *flakyHost) CommitBatch(ctx context.Context, edits []host.Edit) error { return nil } +func (h *flakyHost) StartSequentialWrite() {} +func (h *flakyHost) EndSequentialWrite() {} + +// A transient load error must NOT drop the reload request: accessed is cleared +// only after a successful load, so the next cycle retries. +func TestAccessedSurvivesTransientLoadError(t *testing.T) { + h := &flakyHost{failCount: 2} + m := NewAtomicCounterManager(h, 0, nil) + rid := rdx.IDFromSrcSeqOff(0x1a, 1, 0) + c := m.Counter(rid, 1) // load #1 fails (transient) + _, _ = c.Get(context.Background()) // sets accessed + assert.True(t, c.accessed.Load()) + m.Cycle(context.Background()) // reload #2 fails; accessed must remain set + assert.True(t, c.accessed.Load(), + "accessed must survive a failed reload") + m.Cycle(context.Background()) // reload #3 succeeds; accessed cleared + assert.False(t, c.accessed.Load()) +} diff --git a/counters/sequential_write_test.go b/counters/sequential_write_test.go new file mode 100644 index 0000000..95e95e8 --- /dev/null +++ b/counters/sequential_write_test.go @@ -0,0 +1,78 @@ +package counters_test + +import ( + "context" + "sync" + "testing" + "time" + + "github.com/drpcorg/chotki/protocol" + "github.com/drpcorg/chotki/rdx" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// A Z slot has two local read-modify-write writers: the counter manager's +// flush and "set"-style flows (an ORM Zdelta computed from a loaded value). +// Both must hold the host's sequential-write bracket across their read AND +// commit; without it one lands between the other's read and commit, the two +// ops collide on the revision, and the merge silently drops one (a reset is +// ignored, or flushed usage is erased). +// +// The set goroutine below is the reference pattern for such flows (e.g. the +// keymanager balance/renewal resets): StartSequentialWrite → read the CURRENT +// value → compute Zdelta from it → commit → EndSequentialWrite. Reading +// before the bracket (or from an older snapshot) fixes nothing. +// +// With every RMW bracketed, mixed concurrent adders must converge EXACTLY: +// final = increments + 1000 per set-writer round. Any lost update makes the +// total come up short, so this is a deterministic guard for the protocol. +func TestZSequentialWritersConvergeExactly(t *testing.T) { + ctx := context.Background() + a := openReplica(t, 0x1a, time.Hour) + rid := newCounterObject(t, a) + fid := rid.ToOff(2) // Z field + + c := a.Counter(rid, 2) + _, err := c.Increment(ctx, 0) + require.NoError(t, err) + + const incs = 200 // lock-free +1 increments, flushed periodically + const sets = 20 // bracketed read-modify-write adds of +1000 + var wg sync.WaitGroup + wg.Add(2) + + go func() { // manager path: increments + periodic flushes + defer wg.Done() + for i := 0; i < incs; i++ { + _, e := c.Increment(ctx, 1) + assert.NoError(t, e) + if i%20 == 0 { + a.SyncCounters(ctx) + } + } + }() + + go func() { // set path: read-modify-write inside the bracket + defer wg.Done() + for i := 0; i < sets; i++ { + a.StartSequentialWrite() + _, tlv := a.GetFieldTLV(fid) + total := rdx.Znative(tlv) + _, e := a.EditFieldTLV(ctx, fid, + protocol.Record(rdx.ZCounter, rdx.Zdelta(tlv, total+1000, a.Clock()))) + a.EndSequentialWrite() + assert.NoError(t, e) + } + }() + + wg.Wait() + a.SyncCounters(ctx) // final flush + reload + + want := int64(incs + 1000*sets) + assert.EqualValues(t, want, persistedZ(t, a, rid, 2), + "a lost read-modify-write update: some increment or set was dropped") + got, err := c.Get(ctx) + require.NoError(t, err) + assert.EqualValues(t, want, got) +} diff --git a/examples/plain_object_test.go b/examples/plain_object_test.go index 53f3f1d..b55b126 100644 --- a/examples/plain_object_test.go +++ b/examples/plain_object_test.go @@ -77,4 +77,9 @@ func TestPlainObjectORM(t *testing.T) { assert.Equal(t, rdx.ID0, sidorov3.Group) assert.Equal(t, uint64(124), sidorov3.Score) + // close the DB before the deferred os.RemoveAll: pebble's background + // compactions would otherwise race the removal and fail the test + // after it has already passed + orma3.Close() + _ = a3.Close() } diff --git a/host/host.go b/host/host.go index 984f4e3..25519b5 100644 --- a/host/host.go +++ b/host/host.go @@ -10,6 +10,12 @@ import ( "github.com/drpcorg/chotki/utils" ) +// Edit is an 'E' edit to object Ref, with Body as [F(offset), ] records. +type Edit struct { + Ref rdx.ID + Body protocol.Records +} + type Host interface { ClassFields(cid rdx.ID) (fields classes.Fields, err error) GetFieldTLV(id rdx.ID) (rdt byte, tlv []byte) @@ -20,7 +26,21 @@ type Host interface { Database() *pebble.DB ObjectFieldTLV(fid rdx.ID) (rdt byte, tlv []byte, err error) CommitPacket(ctx context.Context, lit byte, ref rdx.ID, body protocol.Records) (id rdx.ID, err error) + // CommitBatch commits edits in slice order, all-or-nothing, in a single Pebble batch and broadcast. + CommitBatch(ctx context.Context, edits []Edit) (err error) Broadcast(ctx context.Context, records protocol.Records, except string) Drain(ctx context.Context, recs protocol.Records) (err error) + // DrainApplied is Drain plus the count of applied records (0 on error, + // len(recs) on success, since draining is all-or-nothing). + DrainApplied(ctx context.Context, recs protocol.Records) (applied int, err error) + // AbortSyncsVia closes the pending diff-sync points created by the session. + AbortSyncsVia(ctx context.Context, sessionId string) Snapshot() pebble.Reader + // StartSequentialWrite/EndSequentialWrite bracket a read-modify-write (a + // Z-counter flush/"set" reads a slot and writes value@rev+1): held across + // both read and commit, two writers of one slot serialize instead of + // colliding on the revision. Cooperative (commit methods don't take it; the + // read must be inside the bracket), not reentrant. + StartSequentialWrite() + EndSequentialWrite() } diff --git a/indexes/index_manager.go b/indexes/index_manager.go index 0fc1167..0aac6d8 100644 --- a/indexes/index_manager.go +++ b/indexes/index_manager.go @@ -62,10 +62,15 @@ const ( ) type IndexManager struct { - c host.Host - tasksCancels map[string]context.CancelFunc - taskEntries sync.Map - mutexMap sync.Map + c host.Host + tasksCancels map[string]context.CancelFunc + taskEntries sync.Map + // taskWg tracks runReindexTask goroutines; CheckReindexTasks waits on it so + // Close never closes the DB under a running reindex. + taskWg sync.WaitGroup + // hashLocks serializes addHashIndex's read-check-merge, striped by fid. (A + // per-fid map that deletes on unlock let a third writer race a blocked one.) + hashLocks [64]sync.Mutex classCache *lru.Cache[rdx.ID, rdx.ID] hashIndexCache *lru.Cache[string, rdx.ID] } @@ -154,7 +159,7 @@ func parseReindexTasks(key, value []byte) ([]ReindexTask, error) { return tasks, nil } -func (im *IndexManager) AddFullScanIndex(cid rdx.ID, oid rdx.ID, batch *pebble.Batch) error { +func (im *IndexManager) AddFullScanIndex(cid rdx.ID, oid rdx.ID, batch pebble.Writer) error { im.classCache.Add(oid, cid) return batch.Merge( fullScanKey(cid, oid), @@ -277,7 +282,19 @@ func (im *IndexManager) HandleClassUpdateParsed(id rdx.ID, cid rdx.ID, newFields return tasks, nil } -func (im *IndexManager) CheckReindexTasks(ctx context.Context) { +// spawnReindexTask runs a reindex task in a tracked goroutine (see taskWg). +func (im *IndexManager) spawnReindexTask(ctx context.Context, task ReindexTask) { + im.taskWg.Add(1) + go func() { + defer im.taskWg.Done() + im.runReindexTask(ctx, &task) + }() +} + +func (im *IndexManager) CheckReindexTasks(ctx context.Context, period time.Duration) { + // Wait for all in-flight reindex tasks before returning: the Chotki + // waitGroup waits for this worker, and Close closes the DB right after. + defer im.taskWg.Wait() cycle := func() { iter, err := im.c.Database().NewIter(&pebble.IterOptions{ LowerBound: []byte{'I', 'T'}, @@ -307,7 +324,7 @@ func (im *IndexManager) CheckReindexTasks(ctx context.Context) { } ctx, cancel := context.WithCancel(ctx) im.tasksCancels[task.Id()] = cancel - go im.runReindexTask(ctx, &task) + im.spawnReindexTask(ctx, task) } else { // task is working, but we need to restart it, because of external event if rev.(int64) < task.Revision { @@ -318,7 +335,7 @@ func (im *IndexManager) CheckReindexTasks(ctx context.Context) { ctx, cancel := context.WithCancel(ctx) im.tasksCancels[task.Id()] = cancel im.taskEntries.Store(task.Id(), task.Revision) - go im.runReindexTask(ctx, &task) + im.spawnReindexTask(ctx, task) } } case reindexTaskStateInProgress: @@ -332,7 +349,7 @@ func (im *IndexManager) CheckReindexTasks(ctx context.Context) { ctx, cancel := context.WithCancel(ctx) im.taskEntries.Store(task.Id(), task.Revision) im.tasksCancels[task.Id()] = cancel - go im.runReindexTask(ctx, &task) + im.spawnReindexTask(ctx, task) } case reindexTaskStateRemove: // noop @@ -348,20 +365,27 @@ func (im *IndexManager) CheckReindexTasks(ctx context.Context) { } } } + t := time.NewTicker(period) + defer t.Stop() for ctx.Err() == nil { cycle() - time.Sleep(1 * time.Second) + // wait a period, but return promptly on shutdown instead of sleeping + // through it (a long period would otherwise block Close's wg.Wait) + select { + case <-ctx.Done(): + return + case <-t.C: + } } } -func (im *IndexManager) addHashIndex(cid rdx.ID, fid rdx.ID, tlv []byte, batch pebble.Writer) error { - lock, _ := im.mutexMap.LoadOrStore(fid, &sync.Mutex{}) - mt := lock.(*sync.Mutex) +// addHashIndex writes fid's hash-index entry into batch. GetByHash sees only +// committed state, so claims (when non-nil) tracks in-batch values to reject a +// second object claiming one another already did. Pass nil to skip that. +func (im *IndexManager) addHashIndex(cid rdx.ID, fid rdx.ID, tlv []byte, batch pebble.Writer, claims map[string]rdx.ID) error { + mt := &im.hashLocks[(fid.Src()^fid.Pro())%uint64(len(im.hashLocks))] mt.Lock() - defer func() { - mt.Unlock() - im.mutexMap.Delete(fid) - }() + defer mt.Unlock() id, err := im.GetByHash(cid, uint32(fid.Off()), tlv, im.c.Database()) switch err { case nil: @@ -370,10 +394,17 @@ func (im *IndexManager) addHashIndex(cid rdx.ID, fid rdx.ID, tlv []byte, batch p } fallthrough case chotki_errors.ErrObjectUnknown: - cacheKey := append(binary.BigEndian.AppendUint32(cid.Bytes(), uint32(fid.Off())), tlv...) - im.hashIndexCache.Remove(string(cacheKey)) hash := xxhash.Sum64(tlv) key := hashKey(cid, uint32(fid.Off()), hash) + if claims != nil { + ck := string(key) + if prior, ok := claims[ck]; ok && prior != fid.ZeroOff() { + return errors.Join(chotki_errors.ErrHashIndexUinqueConstraintViolation, fmt.Errorf("intra-batch key %s, prior id %s, new id %s", string(tlv), prior.String(), fid.ZeroOff().String())) + } + claims[ck] = fid.ZeroOff() + } + cacheKey := append(binary.BigEndian.AppendUint32(cid.Bytes(), uint32(fid.Off())), tlv...) + im.hashIndexCache.Remove(string(cacheKey)) set := rdx.NewStampedSet[rdx.RdxRid]() set.Add(rdx.RdxRid(fid.ZeroOff())) return batch.Merge( @@ -386,16 +417,23 @@ func (im *IndexManager) addHashIndex(cid rdx.ID, fid rdx.ID, tlv []byte, batch p } } -func (im *IndexManager) OnFieldUpdate(rdt byte, fid, cid rdx.ID, tlv []byte, batch pebble.Writer) error { +// OnFieldUpdate maintains hash indexes for a field write. cid=rdx.BadId is +// resolved from the object's 'O' key. deferred=true means 'O' isn't visible yet, +// so nothing was indexed and the caller must reindex after commit (via IndexObject). +func (im *IndexManager) OnFieldUpdate(rdt byte, fid, cid rdx.ID, tlv []byte, batch pebble.Writer, claims map[string]rdx.ID) (deferred bool, err error) { if !rdx.IsFirst(rdt) { - return nil + return false, nil } if cid == rdx.BadId { var ok bool cid, ok = im.classCache.Get(fid.ZeroOff()) if !ok { - _, tlv := im.c.GetFieldTLV(fid.ZeroOff()) - cid = rdx.IDFromZipBytes(tlv) + _, otlv := im.c.GetFieldTLV(fid.ZeroOff()) + if len(otlv) == 0 { + // object's 'O' key not visible (yet): defer to post-commit + return true, nil + } + cid = rdx.IDFromZipBytes(otlv) if cid != rdx.BadId && cid != rdx.ID0 { im.classCache.Add(fid.ZeroOff(), cid) } @@ -403,7 +441,7 @@ func (im *IndexManager) OnFieldUpdate(rdt byte, fid, cid rdx.ID, tlv []byte, bat } // ID0 class is not indexed if cid == rdx.ID0 { - return nil + return false, nil } fields, err := im.c.ClassFields(cid) if err != nil { @@ -415,20 +453,53 @@ func (im *IndexManager) OnFieldUpdate(rdt byte, fid, cid rdx.ID, tlv []byte, bat Src: im.c.Source(), LastUpdate: time.Now(), } - return batch.Merge(task.Key(), task.Value(), im.c.WriteOptions()) + return false, batch.Merge(task.Key(), task.Value(), im.c.WriteOptions()) } if int(fid.Off()) >= len(fields) { - return nil + return false, nil } field := fields[fid.Off()] if field.Index == classes.HashIndex { _, _, tlv := rdx.ParseFIRST(tlv) - return im.addHashIndex(cid, fid, tlv, batch) + return false, im.addHashIndex(cid, fid, tlv, batch, claims) + } + return false, nil +} + +// IndexObject rebuilds hash-index entries for oid's indexed fields from their +// current merged values. Called after a batch commits, so an edit processed +// before its object's create is picked up without waiting for the reindex worker. +func (im *IndexManager) IndexObject(oid rdx.ID) error { + _, ctlv := im.c.GetFieldTLV(oid.ZeroOff()) + cid := rdx.IDFromZipBytes(ctlv) + if cid == rdx.ID0 || cid == rdx.BadId { + return nil // object's create not applied yet; its own apply will index it + } + fields, err := im.c.ClassFields(cid) + if err != nil { + return err + } + for off := 1; off < len(fields); off++ { + if fields[off].Index != classes.HashIndex { + continue + } + fid := oid.ToOff(uint64(off)) + rdt, tlv, err := im.c.ObjectFieldTLV(fid) + if err != nil || !rdx.IsFirst(rdt) { + continue + } + _, _, val := rdx.ParseFIRST(tlv) + if err := im.addHashIndex(cid, fid, val, im.c.Database(), nil); err != nil { + return err + } } return nil } func (im *IndexManager) runReindexTask(ctx context.Context, task *ReindexTask) { + if ctx.Err() != nil { + return // spawned right before a shutdown/restart: don't touch the DB + } start := time.Now() ReindexCount.WithLabelValues(task.Cid.String(), fmt.Sprintf("%d", task.Field)).Inc() defer im.taskEntries.CompareAndDelete(task.Id(), task.Revision) @@ -505,7 +576,7 @@ func (im *IndexManager) runReindexTask(ctx context.Context, task *ReindexTask) { _, _, tlv = rdx.ParseFIRST(tlv) _, err = im.GetByHash(task.Cid, uint32(fid.Off()), tlv, im.c.Database()) if err == chotki_errors.ErrObjectUnknown { - err = im.addHashIndex(task.Cid, fid, tlv, im.c.Database()) + err = im.addHashIndex(task.Cid, fid, tlv, im.c.Database(), nil) if err != nil { ReindexResults.WithLabelValues(task.Cid.String(), fmt.Sprintf("%d", task.Field), "error", "fail_to_add_hash_index").Inc() im.c.Logger().ErrorCtx(ctx, "failed to add hash index: %s, will restart", err) @@ -529,6 +600,9 @@ func (im *IndexManager) runReindexTask(ctx context.Context, task *ReindexTask) { } defer indexIter.Close() for valid := indexIter.First(); valid; valid = indexIter.Next() { + if ctx.Err() != nil { + return // cancelled (shutdown or task restart): stop before more DB writes + } set := rdx.NewStampedSet[rdx.RdxRid]() err := set.Native(indexIter.Value()) if err != nil { diff --git a/objects.go b/objects.go index 1abbbb8..af243e2 100644 --- a/objects.go +++ b/objects.go @@ -254,6 +254,11 @@ func (cho *Chotki) MapTRField(fid rdx.ID) (themap rdx.MapTR, err error) { // Returns the TLV-encoded value of the object field, given object rdx.ID with offset. func (cho *Chotki) GetFieldTLV(id rdx.ID) (rdt byte, tlv []byte) { + // a concurrent Close nils cho.db; return cleanly to avoid a panic on any + // caller not already guarded by commitMutex/cho.lock. + if cho.db == nil { + return 0, nil + } key := host.OKey(id, 'A') it, err := cho.db.NewIter(&pebble.IterOptions{ LowerBound: []byte{'O'}, diff --git a/packets.go b/packets.go index 281e33f..b7157d7 100644 --- a/packets.go +++ b/packets.go @@ -4,6 +4,7 @@ import ( "errors" "github.com/cockroachdb/pebble" + "github.com/drpcorg/chotki/chotki_errors" "github.com/drpcorg/chotki/host" "github.com/drpcorg/chotki/indexes" "github.com/drpcorg/chotki/protocol" @@ -28,30 +29,51 @@ func (cho *Chotki) UpdateVTree(id, ref rdx.ID, pb *pebble.Batch) (err error) { // During the diff sync it handles the 'D' packets which most of the time contains a single block (look at the replication protocol description). // It does not immediately apply the changes to DB, instead using a batch. // The batch will be applied when we finish the diffsync, when we receive the 'V' packet. -func (cho *Chotki) ApplyD(id, ref rdx.ID, body []byte, batch *pebble.Batch) (err error) { +func (cho *Chotki) ApplyD(id, ref rdx.ID, body []byte, batch pebble.Writer, created *[]rdx.ID, claims map[string]rdx.ID) (err error) { rest := body var rdt byte for len(rest) > 0 && err == nil { var dzip, bare []byte // this is id, but its stored as an offset of ref to save some bytes dzip, rest = protocol.Take('F', rest) + // a missing or truncated 'F' record would otherwise loop forever + // (Take makes no progress on incomplete input) + if dzip == nil { + return rdx.ErrBadPacket + } // It also stored as zigzagged d := rdx.UnzipUint64(dzip) // we now restore original id at := ref.ProPlus(d) rdt, bare, rest = protocol.TakeAny(rest) - // we updated some classes, so dropping cache - if rdt == 'C' { - cho.types.Clear() + if rdt == 0 || bare == nil { + return rdx.ErrBadPacket } + // Don't clear cho.types for a 'C' parcel here: the batch isn't durable + // until 'V', so clearing now would let a concurrent ClassFields re-cache + // the stale schema. The 'V' handler clears it after the batch applies. err = batch.Merge(host.OKey(at, rdt), bare, cho.opts.PebbleWriteOptions) + if err != nil { + // return now so the OnFieldUpdate branch below (nil for non-FIRST + // types) can't swallow the Merge error; this aborts the sync. + return err + } // adding full scan index if the object was created - if err == nil && rdt == 'O' { + if rdt == 'O' { cid := rdx.IDFromZipBytes(bare) + if created != nil { + *created = append(*created, at) // reindexed from merged state after 'V' + } err = cho.IndexManager.AddFullScanIndex(cid, at, batch) } else { // check if we need add other types of indexes - err = cho.IndexManager.OnFieldUpdate(rdt, at, rdx.BadId, bare, batch) + var deferredIdx bool + deferredIdx, err = cho.IndexManager.OnFieldUpdate(rdt, at, rdx.BadId, bare, batch, claims) + if err == nil && deferredIdx && created != nil { + // the object's 'O' rides this same (unapplied) sync batch, so the + // class was unresolvable; reindex from merged state after 'V' + *created = append(*created, at.ZeroOff()) + } } } return @@ -63,6 +85,12 @@ func (cho *Chotki) ApplyH(id, ref rdx.ID, body []byte, batch *pebble.Batch) (err _, rest := protocol.Take('M', body) var vbody []byte vbody, _ = protocol.Take('V', rest) + // Relayed handshakes skip ParseHandshake, so the VV may be missing or + // malformed. Reject rather than merge: Vmerge keeps only the parseable + // prefix of a garbled VV, so the sync would record partial coverage. + if vbody == nil || !rdx.VValid(vbody) { + return chotki_errors.ErrBadHPacket + } err = batch.Merge(host.VKey0, vbody, cho.opts.PebbleWriteOptions) return } @@ -76,14 +104,25 @@ func (cho *Chotki) ApplyV(id, ref rdx.ID, body []byte, batch *pebble.Batch) (err var rec, idb []byte // take block version vector rec, rest = protocol.Take('V', rest) + // stop on a malformed or truncated record: Take makes no progress + // on incomplete input, which would loop forever here + if rec == nil { + return ErrBadVPacket + } // take block id idb, rec = protocol.Take('R', rec) + if idb == nil { + return ErrBadVPacket + } id := rdx.IDFromZipBytes(idb) key := host.VKey(id) if !rdx.VValid(rec) { - err = ErrBadVPacket - } else { - err = batch.Merge(key, rec, cho.opts.PebbleWriteOptions) + // return instead of continuing so the error cannot be + // silently overwritten by a later, well-formed record + return ErrBadVPacket + } + if err = batch.Merge(key, rec, cho.opts.PebbleWriteOptions); err != nil { + return err } } return @@ -131,7 +170,7 @@ func (cho *Chotki) ApplyC(id, ref rdx.ID, body []byte, batch *pebble.Batch, call // Then it goes through the rest of the fields encoded as TLV. The only transformation it does: // it sets the current replica src id for FIRST/MEL types, because historically they are not set // when creating those fields (for convinience?) -func (cho *Chotki) ApplyOY(lot byte, id, ref rdx.ID, body []byte, batch *pebble.Batch) (err error) { +func (cho *Chotki) ApplyOY(lot byte, id, ref rdx.ID, body []byte, batch *pebble.Batch, claims map[string]rdx.ID) (err error) { // creating 'O' field, ref is class rdx.ID err = batch.Merge( host.OKey(id, lot), @@ -172,7 +211,8 @@ func (cho *Chotki) ApplyOY(lot byte, id, ref rdx.ID, body []byte, batch *pebble. cho.opts.PebbleWriteOptions) rest = rest[rlen:] if err == nil { - err = cho.IndexManager.OnFieldUpdate(lit, fid, ref, rebar, batch) + // cid (ref) is known here, so the update is never deferred + _, err = cho.IndexManager.OnFieldUpdate(lit, fid, ref, rebar, batch, claims) } } if err == nil { @@ -190,7 +230,9 @@ var ErrOffsetOpId = errors.New("op id is offset") // Edits obkject fields. Unlike ApplyOY, it does not assume that we update whole object, // as we can update individual fields. // It also sets the current replica src id for FIRST/MEL types. Otherwise its just merges bytes into the batch. -func (cho *Chotki) ApplyE(id, r rdx.ID, body []byte, batch *pebble.Batch, calls *[]CallHook) (err error) { +// If the object's class isn't resolvable yet, the id is appended to created for +// the caller to reindex after the batch commits. +func (cho *Chotki) ApplyE(id, r rdx.ID, body []byte, batch *pebble.Batch, calls *[]CallHook, created *[]rdx.ID, claims map[string]rdx.ID) (err error) { // we either supply id of the object (0 offset) or ref should be an id of the object if id.Off() != 0 || r.Off() != 0 { return ErrOffsetOpId @@ -209,6 +251,10 @@ func (cho *Chotki) ApplyE(id, r rdx.ID, body []byte, batch *pebble.Batch, calls return ErrBadEPacket } lit, bare, rest = protocol.TakeAny(rest) + // a truncated value makes no progress; fail now (as ApplyD/ApplyV do) + if lit == 0 || bare == nil { + return ErrBadEPacket + } // setting current replica src id for FIRST/MEL types switch lit { case 'F', 'I', 'R', 'S', 'T': @@ -230,7 +276,12 @@ func (cho *Chotki) ApplyE(id, r rdx.ID, body []byte, batch *pebble.Batch, calls cho.opts.PebbleWriteOptions) if err == nil { - err = cho.IndexManager.OnFieldUpdate(lit, fid, rdx.BadId, rebar, batch) + var deferredIdx bool + deferredIdx, err = cho.IndexManager.OnFieldUpdate(lit, fid, rdx.BadId, rebar, batch, claims) + if err == nil && deferredIdx && created != nil { + // object not visible yet: reindex from merged state post-commit + *created = append(*created, r) + } } // hooks are used for REPL sometimes (or where used), otherwise unused hook, ok := cho.hooks.Load(fid) diff --git a/packets_apply_test.go b/packets_apply_test.go new file mode 100644 index 0000000..e4f7df1 --- /dev/null +++ b/packets_apply_test.go @@ -0,0 +1,45 @@ +package chotki + +import ( + "errors" + "testing" + + "github.com/cockroachdb/pebble" + "github.com/drpcorg/chotki/protocol" + "github.com/drpcorg/chotki/rdx" + "github.com/stretchr/testify/require" +) + +// failMergeWriter wraps a real batch but fails Merge for object ('O'-prefixed) +// keys, so we can verify ApplyD surfaces the Merge error instead of the +// else-branch OnFieldUpdate call reassigning err to nil. +type failMergeWriter struct { + *pebble.Batch +} + +func (w failMergeWriter) Merge(key, value []byte, o *pebble.WriteOptions) error { + if len(key) > 0 && key[0] == 'O' { + return errors.New("injected merge failure") + } + return w.Batch.Merge(key, value, o) +} + +func TestApplyDReturnsMergeError(t *testing.T) { + dirs, clear := testdirs(0x71) + defer clear() + a, err := Open(dirs[0], Options{Src: 0x71, Name: "A"}) + require.NoError(t, err) + defer a.Close() + + batch := a.db.NewBatch() + defer batch.Close() + ref := rdx.IDFromSrcSeqOff(0x71, 1, 0) + // one D-parcel: F(offset) + an 'S' (FIRST) record, so the else-branch runs + body := append( + protocol.Record('F', rdx.ZipUint64(1)), + protocol.Record('S', rdx.Stlv("x"))..., + ) + var created []rdx.ID + err = a.ApplyD(ref, ref, body, failMergeWriter{batch}, &created, nil) + require.Error(t, err, "ApplyD must return the batch.Merge error, not swallow it") +} diff --git a/replication/README.md b/replication/README.md index 8c1da77..8b3b018 100644 --- a/replication/README.md +++ b/replication/README.md @@ -150,6 +150,24 @@ The algorithm of diff sync is as follows: Important note that during diff sync we also broadcast all 'D' and 'H' packets to all other replicas. Imagine there are 3 replicas: A <-> B <-> C. +The exact rebroadcast rule is: draining a batch of records is all-or-nothing (drop-on-error), and a +replica relays a batch to all its other sessions only when the batch fully persisted in its own DB — +except the trailing 'B' (bye) record, which is scoped to this session (network read batching can +coalesce data records with the peer's closing 'B' into one batch). On a mid-batch error (e.g. +ErrSyncUnknown on a stale sync point) nothing is applied and nothing is relayed; the erroring session +dies and the data is re-delivered by the diff sync after reconnect. The applied ⇒ relayed direction is +the correctness-critical one: applied-but-not-relayed records would never reach downstream replicas at +all — live records are not re-sent, and future diff syncs would skip them because the middle replica +already has them — leaving the tree silently diverged. A formal model of this protocol (including a +reproduction of that scenario) lives in the `tla/` directory. + +A related lifetime rule: a sync point (the pebble batch accumulating a diff) is bound to the replication +session whose 'H' record created it, and is aborted when that session closes (Syncer.SessionId / +Chotki.AbortSyncsVia). The remaining 'D'/'V' packets of a diff always travel through that same session, so +once it is gone the batch can never be completed legitimately; without the abort, a 'V' relayed through a +newer connection could apply the staged handshake version vector without the data records that died with +the old connection — permanent, silent data loss (see tla/README.md, bug 3). + - Let's say A, B are live syncing and C is just connected to B. - If we do not broadcast 'D' packets, then B and C will sync, however if there is something in C, that A haven't seen it will not be synced, until diff sync between A and B. - But if we broadcast 'D' and 'H' packets, we basically open syncing session between all upstream replicas and C, so @@ -163,3 +181,19 @@ But it simplified protocol a lot. After diff sync, we now can process all updates that were accumulated in the queue and continue process them as we go. When we receive a bunch of records during live sync, we apply them to the DB immediately and also broadcast then to all other replicas. Due to the fact that currently chotki only allows tree replication structures, we know that we can safely send events to all connected replicas without fear of loops. + +## Sync-point displacement assumes a tree topology + +A new handshake from an origin displaces that origin's older, still-unfinished +diff sync ("allow only 1 diff sync per src", `chotki.go` `drain` 'H' case). This +displacement is only safe under a **tree topology** (exactly one path per +origin): a handshake and its diff always travel the same single session, so a +"new handshake" genuinely supersedes the previous one. In a cyclic topology the +same origin could be reached through two sessions at once, and a relayed +handshake arriving on one could evict the live direct diff on the other, +flapping the session with `ErrSyncUnknown`. To keep that from +happening even if a stray relayed handshake appears, displacement is restricted +to sync points that are either from the **same session** (a genuine restart) or +a **strictly older snapshot** (`key.Less(incoming)`); an older/equal handshake +from a different session leaves a live diff untouched. Cyclic topologies remain +unsupported. diff --git a/replication/sync.go b/replication/sync.go index 0740c17..aa186dd 100644 --- a/replication/sync.go +++ b/replication/sync.go @@ -29,10 +29,35 @@ var version string = fmt.Sprintf("%d", time.Now().Unix()) type SyncHost interface { protocol.Drainer + // DrainApplied is Drain plus the count of applied records (0 on error, + // len(recs) on success, since draining is all-or-nothing). + DrainApplied(ctx context.Context, recs protocol.Records) (int, error) + // AbortSyncsVia closes and removes the pending diff-sync points + // created by the given replication session (see Syncer.SessionId). + AbortSyncsVia(ctx context.Context, sessionId string) Snapshot() pebble.Reader Broadcast(ctx context.Context, records protocol.Records, except string) } +type sessionIdCtxKey struct{} + +// WithSessionId marks ctx with the id of the replication session that is +// draining the records; the host binds the sync points (pending diff +// batches) created by those records to the session lifetime, so they can +// be aborted when the session ends (SyncHost.AbortSyncsVia). +func WithSessionId(ctx context.Context, id string) context.Context { + return context.WithValue(ctx, sessionIdCtxKey{}, id) +} + +// SessionIdFromCtx extracts the replication session id set by WithSessionId, +// or "" if none. +func SessionIdFromCtx(ctx context.Context) string { + if v, ok := ctx.Value(sessionIdCtxKey{}).(string); ok { + return v + } + return "" +} + type SyncMode byte const ( @@ -134,6 +159,30 @@ type Syncer struct { pingTimer *time.Timer pingStage atomic.Int32 lctx atomic.Pointer[context.Context] + + // guards snap, vvit and ffit: the feed loop and Close may touch the + // snapshot and its iterators concurrently + snapLock sync.Mutex + // set under snapLock when a snapshot/iterator close failed mid-session, so + // Close keeps that gauge series alive as the leak signal instead of deleting it + snapCloseFailed bool + iterCloseFailed bool + sessionOnce sync.Once + sessionUid string +} + +// SessionId returns a unique identifier of this replication session; the +// host uses it to bind the sync points this session creates to its +// lifetime (SyncHost.AbortSyncsVia). +func (sync *Syncer) SessionId() string { + sync.sessionOnce.Do(func() { + if id, err := uuid.NewV7(); err == nil { + sync.sessionUid = id.String() + } else { + sync.sessionUid = fmt.Sprintf("%s-%d", sync.Name, time.Now().UnixNano()) + } + }) + return sync.sessionUid } func (sync *Syncer) withDefaultArgs(reset bool) context.Context { @@ -158,23 +207,39 @@ func (sync *Syncer) LogCtx(ctx context.Context) context.Context { func (sync *Syncer) Close() error { sync.SetFeedState(context.Background(), SendNone) + // The id label is unique per connection, so a series left behind after the + // session ends leaks until process restart. Drop the state series + // unconditionally (even on the nil-Host early return below); the + // snapshot/iterator series are dropped further down only when their close + // succeeded, so a real resource leak stays visible. + defer SessionsStates.DeleteLabelValues(sync.Name, "feed", version) + defer SessionsStates.DeleteLabelValues(sync.Name, "drain", version) + if sync.Host == nil { return utils.ErrClosed } - sync.lock.Lock() - defer sync.lock.Unlock() + // Abort sync points this session created: their 'D'/'V' travel through + // this connection, so once it's gone the batch can't complete, and a 'V' + // relayed later would apply the handshake VV without the data (see tla/). + sync.Host.AbortSyncsVia(sync.LogCtx(context.Background()), sync.SessionId()) + + sync.snapLock.Lock() + defer sync.snapLock.Unlock() + closesnapshot := !sync.snapCloseFailed if sync.snap != nil { if err := sync.snap.Close(); err != nil { + closesnapshot = false sync.Log.ErrorCtx(sync.LogCtx(context.Background()), "failed closing snapshot", "err", err.Error()) - } else { - OpenedSnapshots.WithLabelValues(sync.Name, version).Set(0) } sync.snap = nil } + if closesnapshot { + OpenedSnapshots.DeleteLabelValues(sync.Name, version) + } - closediterators := true + closediterators := !sync.iterCloseFailed if sync.ffit != nil { if err := sync.ffit.Close(); err != nil { @@ -192,7 +257,7 @@ func (sync *Syncer) Close() error { sync.vvit = nil } if closediterators { - OpenedIterators.WithLabelValues(sync.Name, version).Set(0) + OpenedIterators.DeleteLabelValues(sync.Name, version) } sync.Log.InfoCtx(sync.LogCtx(context.Background()), fmt.Sprintf("sync: connection %s closed: %v\n", sync.Name, sync.reason)) @@ -225,14 +290,21 @@ func (sync *Syncer) GetDrainState() SyncState { func (sync *Syncer) Feed(ctx context.Context) (recs protocol.Records, err error) { SessionsStates.WithLabelValues(sync.Name, "feed", version).Set(float64(sync.GetFeedState())) - // other side closed the connection already + // The peer's bye only means it has nothing more to send; it keeps + // applying our records until the connection closes. Finish our own + // handshake/diff phase first (cutting it short would leave the peer a + // staged batch with no 'V', silently missing promised data), then wind down. if sync.GetDrainState() == SendNone { - sync.SetFeedState(ctx, SendNone) + if fs := sync.GetFeedState(); fs != SendHandshake && fs != SendDiff { + sync.SetFeedState(ctx, SendNone) + } } switch sync.GetFeedState() { case SendHandshake: recs, err = sync.FeedHandshake() - sync.SetFeedState(ctx, SendDiff) + if err == nil { + sync.SetFeedState(ctx, SendDiff) + } case SendDiff: ctx, cancel := context.WithCancel(ctx) @@ -255,9 +327,11 @@ func (sync *Syncer) Feed(ctx context.Context) (recs protocol.Records, err error) sync.SetFeedState(ctx, SendEOF) } + sync.snapLock.Lock() if sync.snap != nil { err = sync.snap.Close() if err != nil { + sync.snapCloseFailed = true sync.Log.ErrorCtx(sync.LogCtx(ctx), "sync: failed closing snapshot", "err", err) } else { OpenedSnapshots.WithLabelValues(sync.Name, version).Set(0) @@ -265,18 +339,25 @@ func (sync *Syncer) Feed(ctx context.Context) (recs protocol.Records, err error) sync.snap = nil err = nil } + sync.snapLock.Unlock() } case SendPing: recs = protocol.Records{ protocol.Record('P', rdx.Stlv(PingVal)), } sync.SetFeedState(ctx, SendLive) - sync.pingTimer.Stop() + sync.pingStage.Store(int32(WaitingForPing)) + // pingTimer is shared with resetPingTimer (drain side), so it + // must only be touched under the lock + sync.lock.Lock() + if sync.pingTimer != nil { + sync.pingTimer.Stop() + } sync.pingTimer = time.AfterFunc(sync.PingWait, func() { sync.pingStage.Store(int32(PingBroken)) sync.Log.ErrorCtx(sync.LogCtx(ctx), "sync: peer did not respond to ping") }) - sync.pingStage.Store(int32(WaitingForPing)) + sync.lock.Unlock() case SendPong: recs = protocol.Records{ protocol.Record('P', rdx.Stlv(PongVal)), @@ -301,15 +382,18 @@ func (sync *Syncer) Feed(ctx context.Context) (recs protocol.Records, err error) protocol.TinyRecord('T', sync.snaplast.ZipBytes()), reason, )} + sync.snapLock.Lock() if sync.snap != nil { err = sync.snap.Close() if err != nil { + sync.snapCloseFailed = true sync.Log.ErrorCtx(sync.LogCtx(ctx), "sync: failed closing snapshot", "error", err.Error()) } else { OpenedSnapshots.WithLabelValues(sync.Name, version).Set(0) } sync.snap = nil } + sync.snapLock.Unlock() sync.SetFeedState(ctx, SendNone) case SendNone: @@ -317,11 +401,14 @@ func (sync *Syncer) Feed(ctx context.Context) (recs protocol.Records, err error) if wait == 0 { wait = time.Second } - timer := time.AfterFunc(wait, func() { - sync.SetDrainState(ctx, SendNone) - }) - <-sync.WaitDrainState(context.Background(), SendNone) - timer.Stop() + // give the peer up to WaitUntilNone to drain our 'B' and close + // first, but bound the wait with a timeout instead of forcing + // the drain state via a one-shot timer: a Drain racing with + // Close could move the drain state backwards after that timer + // had already fired, leaving this wait blocked forever + nctx, cancel := context.WithTimeout(ctx, wait) + <-sync.WaitDrainState(nctx, SendNone) + cancel() err = io.EOF } @@ -329,6 +416,9 @@ func (sync *Syncer) Feed(ctx context.Context) (recs protocol.Records, err error) } func (sync *Syncer) FeedHandshake() (vv protocol.Records, err error) { + sync.snapLock.Lock() + defer sync.snapLock.Unlock() + sync.snap = sync.Host.Snapshot() OpenedSnapshots.WithLabelValues(sync.Name, version).Set(1) @@ -439,6 +529,9 @@ func (sync *Syncer) nextBlockDiff() (bool, rdx.VV, error) { } func (sync *Syncer) FeedBlockDiff(ctx context.Context) (diff protocol.Records, err error) { + sync.snapLock.Lock() + defer sync.snapLock.Unlock() + hasChanges, sendvv, cerr := sync.nextBlockDiff() if cerr != nil { return nil, cerr @@ -488,6 +581,9 @@ func (sync *Syncer) FeedBlockDiff(ctx context.Context) (diff protocol.Records, e } func (sync *Syncer) FeedDiffVV(ctx context.Context) (vv protocol.Records, err error) { + sync.snapLock.Lock() + defer sync.snapLock.Unlock() + protocol.CloseHeader(sync.vpack, 5) vv = append(vv, sync.vpack) sync.vpack = nil @@ -510,6 +606,8 @@ func (sync *Syncer) FeedDiffVV(ctx context.Context) (vv protocol.Records, err er } if closediterators { OpenedIterators.WithLabelValues(sync.Name, version).Set(0) + } else { + sync.iterCloseFailed = true } return } @@ -535,13 +633,23 @@ func (sync *Syncer) SetDrainState(ctx context.Context, state SyncState) { } func (sync *Syncer) WaitDrainState(ctx context.Context, state SyncState) chan SyncState { - res := make(chan SyncState) + // buffered so the waiter goroutine never blocks (and leaks) when the + // caller abandons the wait (e.g. on timeout) + res := make(chan SyncState, 1) + // derive a cancelable context so the watcher goroutine below always + // terminates, even for non-cancellable parent contexts + ctx, cancel := context.WithCancel(ctx) go func() { <-ctx.Done() + // take the lock so the wakeup cannot slip between the waiter's + // ctx check and its cond.Wait() + sync.lock.Lock() sync.cond.Broadcast() + sync.lock.Unlock() }() go func() { defer close(res) + defer cancel() sync.lock.Lock() defer sync.lock.Unlock() if sync.cond.L == nil { @@ -585,25 +693,39 @@ func (sync *Syncer) resetPingTimer() { } func (sync *Syncer) processPings(recs protocol.Records) protocol.Records { - for i, rec := range recs { - if protocol.Lit(rec) == 'P' { - body, _ := protocol.Take('P', rec) - if len(recs) > i+1 { - recs = append(recs[:i], recs[i+1:]...) - } else { - recs = recs[:i] - } - switch rdx.Snative(body) { - case PingVal: - sync.Log.InfoCtx(sync.LogCtx(context.Background()), "ping received") - // go to pong state next time - sync.pingStage.Store(int32(Pong)) - case PongVal: - sync.Log.InfoCtx(sync.LogCtx(context.Background()), "pong received") - } + // filter 'P' records out in place: they're session-scoped and must not + // reach the DB or be relayed. + filtered := recs[:0] + for _, rec := range recs { + if protocol.Lit(rec) != 'P' { + filtered = append(filtered, rec) + continue + } + body, _ := protocol.Take('P', rec) + switch rdx.Snative(body) { + case PingVal: + sync.Log.InfoCtx(sync.LogCtx(context.Background()), "ping received") + // go to pong state next time + sync.pingStage.Store(int32(Pong)) + case PongVal: + sync.Log.InfoCtx(sync.LogCtx(context.Background()), "pong received") } } - return recs + return filtered +} + +// relayApplied rebroadcasts the applied records, minus a trailing session-scoped +// 'B' (bye). Draining is all-or-nothing, so applied is 0 (dropped — resync +// re-delivers) or len(recs); a persisted batch MUST relay, or downstream never +// gets it (live records aren't re-sent and diff syncs would skip them). +func (sync *Syncer) relayApplied(ctx context.Context, recs protocol.Records, applied int) { + relay := recs[:applied] + if LastLit(relay) == 'B' { + relay = relay[:len(relay)-1] + } + if len(relay) > 0 { + sync.Host.Broadcast(sync.LogCtx(ctx), relay, sync.Name) + } } func (sync *Syncer) Drain(ctx context.Context, recs protocol.Records) (err error) { @@ -613,15 +735,30 @@ func (sync *Syncer) Drain(ctx context.Context, recs protocol.Records) (err error SessionsStates.WithLabelValues(sync.Name, "drain", version).Set(float64(sync.GetDrainState())) recs = sync.processPings(recs) + if len(recs) == 0 { + // A ping-only batch is normal once live, but before the handshake it's a + // protocol error: a ping-only peer would keep the session alive forever + // without ever handshaking. + if sync.GetDrainState() == SendHandshake { + return chotki_errors.ErrBadHPacket + } + // the batch contained pings only; there is nothing to drain, + // relay or change state upon + if sync.Mode&SyncLive != 0 { + sync.resetPingTimer() + } + return nil + } + + // mark the records with this session's id so the sync points they + // create die together with the session (AbortSyncsVia in Close) + hctx := WithSessionId(sync.LogCtx(ctx), sync.SessionId()) switch sync.GetDrainState() { case SendHandshake: - if len(recs) == 0 { - return chotki_errors.ErrBadHPacket - } err = sync.DrainHandshake(recs[0:1]) if err == nil { - err = sync.Host.Drain(sync.LogCtx(ctx), recs[0:1]) + err = sync.Host.Drain(hctx, recs[0:1]) } if err != nil { return @@ -635,11 +772,9 @@ func (sync *Syncer) Drain(ctx context.Context, recs protocol.Records) (err error fallthrough case SendDiff: - broadcast := true lit := LastLit(recs) if lit != 'D' && lit != 'V' { if lit == 'B' { - broadcast = false sync.SetDrainState(ctx, SendNone) } else { sync.SetDrainState(ctx, SendLive) @@ -648,10 +783,9 @@ func (sync *Syncer) Drain(ctx context.Context, recs protocol.Records) (err error if sync.Mode&SyncLive != 0 { sync.resetPingTimer() } - err = sync.Host.Drain(sync.LogCtx(ctx), recs) - if err == nil && broadcast { - sync.Host.Broadcast(sync.LogCtx(ctx), recs, sync.Name) - } + var applied int + applied, err = sync.Host.DrainApplied(hctx, recs) + sync.relayApplied(ctx, recs, applied) case SendLive: sync.resetPingTimer() @@ -659,10 +793,9 @@ func (sync *Syncer) Drain(ctx context.Context, recs protocol.Records) (err error if lit == 'B' { sync.SetDrainState(ctx, SendNone) } - err = sync.Host.Drain(sync.LogCtx(ctx), recs) - if err == nil && lit != 'B' { - sync.Host.Broadcast(sync.LogCtx(ctx), recs, sync.Name) - } + var applied int + applied, err = sync.Host.DrainApplied(hctx, recs) + sync.relayApplied(ctx, recs, applied) case SendPong, SendPing: panic("chotki: unacceptable sync-state") diff --git a/replication/sync_metrics_test.go b/replication/sync_metrics_test.go new file mode 100644 index 0000000..147d43e --- /dev/null +++ b/replication/sync_metrics_test.go @@ -0,0 +1,140 @@ +package replication + +import ( + "context" + "errors" + "io" + "log/slog" + "testing" + + "github.com/cockroachdb/pebble" + "github.com/drpcorg/chotki/protocol" + "github.com/drpcorg/chotki/utils" + "github.com/prometheus/client_golang/prometheus" + "github.com/stretchr/testify/require" +) + +type fakeSyncHost struct{} + +func (f *fakeSyncHost) Drain(ctx context.Context, recs protocol.Records) error { return nil } +func (f *fakeSyncHost) DrainApplied(ctx context.Context, recs protocol.Records) (int, error) { + return len(recs), nil +} +func (f *fakeSyncHost) AbortSyncsVia(ctx context.Context, sessionId string) {} +func (f *fakeSyncHost) Snapshot() pebble.Reader { return nil } +func (f *fakeSyncHost) Broadcast(ctx context.Context, recs protocol.Records, except string) { +} + +// countSeries returns how many series of the vector carry the given session id label. +func countSeries(t *testing.T, vec *prometheus.GaugeVec, id string) int { + t.Helper() + reg := prometheus.NewPedanticRegistry() + require.NoError(t, reg.Register(vec)) + mfs, err := reg.Gather() + require.NoError(t, err) + count := 0 + for _, mf := range mfs { + for _, m := range mf.GetMetric() { + for _, l := range m.GetLabel() { + if l.GetName() == "id" && l.GetValue() == id { + count++ + } + } + } + } + return count +} + +func TestCloseRemovesSessionMetricSeries(t *testing.T) { + name := "listen:test-close-removes-series" + syn := &Syncer{ + Name: name, + Host: &fakeSyncHost{}, + Log: utils.NewDefaultLogger(slog.LevelError), + } + + // Simulate what a live session does: state gauges for feed/drain, + // snapshot and iterator gauges from the handshake. + syn.SetFeedState(context.Background(), SendLive) + syn.SetDrainState(context.Background(), SendLive) + OpenedSnapshots.WithLabelValues(name, version).Set(1) + OpenedIterators.WithLabelValues(name, version).Set(1) + + require.Equal(t, 2, countSeries(t, SessionsStates, name), "feed+drain series must exist before Close") + require.Equal(t, 1, countSeries(t, OpenedSnapshots, name)) + require.Equal(t, 1, countSeries(t, OpenedIterators, name)) + + require.NoError(t, syn.Close()) + + require.Equal(t, 0, countSeries(t, SessionsStates, name), "session state series must be deleted on Close") + require.Equal(t, 0, countSeries(t, OpenedSnapshots, name), "snapshot series must be deleted on clean Close") + require.Equal(t, 0, countSeries(t, OpenedIterators, name), "iterator series must be deleted on clean Close") +} + +type failingReader struct{} + +func (f *failingReader) Get(key []byte) (value []byte, closer io.Closer, err error) { + return nil, nil, pebble.ErrNotFound +} +func (f *failingReader) NewIter(o *pebble.IterOptions) (*pebble.Iterator, error) { + return nil, errors.New("not implemented") +} +func (f *failingReader) Close() error { return errors.New("snapshot close failed") } + +func TestCloseKeepsSnapshotSeriesWhenSnapshotCloseFails(t *testing.T) { + name := "listen:test-close-snap-fail" + syn := &Syncer{ + Name: name, + Host: &fakeSyncHost{}, + Log: utils.NewDefaultLogger(slog.LevelError), + } + syn.snap = &failingReader{} + OpenedSnapshots.WithLabelValues(name, version).Set(1) + t.Cleanup(func() { OpenedSnapshots.DeleteLabelValues(name, version) }) + syn.SetFeedState(context.Background(), SendLive) + + require.NoError(t, syn.Close()) + + require.Equal(t, 1, countSeries(t, OpenedSnapshots, name), "failed snapshot close must keep the leak signal") + require.Equal(t, 0, countSeries(t, SessionsStates, name)) +} + +func TestCloseKeepsSeriesAfterMidSessionCloseFailure(t *testing.T) { + name := "listen:test-close-mid-session-fail" + syn := &Syncer{ + Name: name, + Host: &fakeSyncHost{}, + Log: utils.NewDefaultLogger(slog.LevelError), + } + // Simulate Feed()/FeedDiffVV() having failed to close the snapshot and + // iterators mid-session: the handles are nil'ed there, but the gauge + // series stay at 1 as the leak signal. + syn.snapCloseFailed = true + syn.iterCloseFailed = true + OpenedSnapshots.WithLabelValues(name, version).Set(1) + OpenedIterators.WithLabelValues(name, version).Set(1) + t.Cleanup(func() { + OpenedSnapshots.DeleteLabelValues(name, version) + OpenedIterators.DeleteLabelValues(name, version) + }) + + require.NoError(t, syn.Close()) + + require.Equal(t, 1, countSeries(t, OpenedSnapshots, name), "mid-session snapshot close failure must keep the leak signal") + require.Equal(t, 1, countSeries(t, OpenedIterators, name), "mid-session iterator close failure must keep the leak signal") +} + +func TestCloseWithNilHostStillRemovesSessionSeries(t *testing.T) { + name := "listen:test-close-nil-host" + syn := &Syncer{ + Name: name, + Log: utils.NewDefaultLogger(slog.LevelError), + } + + syn.SetFeedState(context.Background(), SendLive) + require.Equal(t, 1, countSeries(t, SessionsStates, name)) + + require.Error(t, syn.Close()) // nil Host reports ErrClosed, but must not leak series + + require.Equal(t, 0, countSeries(t, SessionsStates, name), "series must be deleted even on the nil-Host path") +} diff --git a/replication/sync_test.go b/replication/sync_test.go new file mode 100644 index 0000000..7fc2ebd --- /dev/null +++ b/replication/sync_test.go @@ -0,0 +1,102 @@ +package replication + +import ( + "context" + "log/slog" + "runtime" + "testing" + "time" + + "github.com/drpcorg/chotki/chotki_errors" + "github.com/drpcorg/chotki/protocol" + "github.com/drpcorg/chotki/rdx" + "github.com/drpcorg/chotki/utils" + "github.com/stretchr/testify/require" +) + +func testSyncer() *Syncer { + return &Syncer{ + Name: "test", + Log: utils.NewDefaultLogger(slog.LevelError), + } +} + +// processPings used to remove 'P' records while ranging over the same +// slice, which skipped the record that followed a removed one: in a +// coalesced [ping, X] batch, X escaped ping processing and a stray 'P' +// could survive into the relayed records. +func TestProcessPingsFiltersAllPingsAndKeepsTheRest(t *testing.T) { + sync := testSyncer() + + ping := protocol.Record('P', rdx.Stlv(PingVal)) + pong := protocol.Record('P', rdx.Stlv(PongVal)) + e1 := protocol.Record('E', []byte("one")) + e2 := protocol.Record('E', []byte("two")) + + out := sync.processPings(protocol.Records{e1, ping, e2, pong}) + require.Equal(t, protocol.Records{e1, e2}, out) + require.Equal(t, int32(Pong), sync.pingStage.Load(), + "a received ping must schedule a pong") + + sync = testSyncer() + out = sync.processPings(protocol.Records{ping, pong}) + require.Empty(t, out, "consecutive pings must all be filtered") +} + +// A peer that sends only pings before completing the handshake must not keep the +// session alive: a ping-only batch in the SendHandshake state is a protocol +// error. +func TestPingOnlyBeforeHandshakeIsError(t *testing.T) { + sync := testSyncer() + sync.SetDrainState(context.Background(), SendHandshake) + + ping := protocol.Record('P', rdx.Stlv(PingVal)) + err := sync.Drain(context.Background(), protocol.Records{ping}) + require.ErrorIs(t, err, chotki_errors.ErrBadHPacket, + "a ping-only batch before the handshake must be rejected") +} + +// WaitDrainState used to leak its watcher goroutine forever when given a +// non-cancellable context (Feed's SendNone path passes +// context.Background()), one goroutine per closed session. +func TestWaitDrainStateDoesNotLeakGoroutines(t *testing.T) { + sync := testSyncer() + + before := runtime.NumGoroutine() + for i := 0; i < 50; i++ { + ch := sync.WaitDrainState(context.Background(), SendDiff) + sync.SetDrainState(context.Background(), SendDiff) + <-ch + sync.SetDrainState(context.Background(), SendHandshake) + } + + deadline := time.Now().Add(3 * time.Second) + for runtime.NumGoroutine() > before+5 && time.Now().Before(deadline) { + time.Sleep(10 * time.Millisecond) + } + require.LessOrEqual(t, runtime.NumGoroutine(), before+5, + "WaitDrainState goroutines must terminate once the wait completes") +} + +// The result channel is buffered now: a caller that abandons the wait on +// timeout (Feed's SendDiff select) must not strand the waiter goroutine +// on the send. +func TestWaitDrainStateAbandonedWaitDoesNotLeak(t *testing.T) { + sync := testSyncer() + + before := runtime.NumGoroutine() + for i := 0; i < 50; i++ { + ctx, cancel := context.WithCancel(context.Background()) + _ = sync.WaitDrainState(ctx, SendDiff) // never read + sync.SetDrainState(context.Background(), SendDiff) + cancel() + sync.SetDrainState(context.Background(), SendHandshake) + } + + deadline := time.Now().Add(3 * time.Second) + for runtime.NumGoroutine() > before+5 && time.Now().Before(deadline) { + time.Sleep(10 * time.Millisecond) + } + require.LessOrEqual(t, runtime.NumGoroutine(), before+5, + "abandoned WaitDrainState waiters must terminate") +} diff --git a/tla/.gitignore b/tla/.gitignore new file mode 100644 index 0000000..a3a9ce4 --- /dev/null +++ b/tla/.gitignore @@ -0,0 +1 @@ +states/ diff --git a/tla/ChotkiSync.tla b/tla/ChotkiSync.tla new file mode 100644 index 0000000..bc94f2c --- /dev/null +++ b/tla/ChotkiSync.tla @@ -0,0 +1,820 @@ +---------------------------- MODULE ChotkiSync ---------------------------- +(***************************************************************************) +(* A TLA+ specification of the Chotki replication (sync) protocol, as *) +(* implemented in: *) +(* *) +(* replication/sync.go -- Syncer feed/drain state machines *) +(* chotki.go -- Chotki.drain / Broadcast / CommitPacket *) +(* packets.go -- ApplyH / ApplyD / ApplyV / ApplyE *) +(* network/peer.go -- read-side batching (record coalescing) *) +(* *) +(* -------------------------- ABSTRACTIONS ------------------------------ *) +(* *) +(* Replica DB state. A replica's pebble DB is abstracted to: *) +(* store[r] : the set of operations (op = [src, seq, obj]) applied; *) +(* gvv[r] : the global version vector VKey0 (src -> max seq seen); *) +(* bvv[r] : per-object version vector, VKey(block). A sync "block" *) +(* is modelled as a single object (1 block == 1 object); *) +(* this keeps the block/global VV distinction, which is *) +(* what the diff algorithm actually depends on. *) +(* *) +(* Operations. All mutation packets ('O','E','C','Y', ...) behave *) +(* identically w.r.t. the sync protocol: they are stamped with an id *) +(* (src, seq), update the object they touch, and update both VVs. They *) +(* are modelled as a single record type "E" (a live op). *) +(* *) +(* Diff filtering. FeedBlockDiff sends, for a block with changes, either *) +(* the full object value (when the peer has not seen the object's *) +(* creation) or rdx.Xdiff(value, sendvv). Both cases amount to "all *) +(* op-stamps in the block newer than the peer's global VV"; the model *) +(* sends exactly the set {op : op.seq > peervv[op.src]} for the object. *) +(* *) +(* Network. Each established connection is a pair of directed, ordered, *) +(* reliable channels. network/peer.go accumulates bytes and hands the *) +(* Syncer *batches* of records (keepRead), so a Drain call receives an *) +(* arbitrary non-empty prefix of the channel -- this coalescing is *) +(* essential: several protocol decisions (LastLit) look only at the last *) +(* record of a batch. *) +(* *) +(* Sync modes. chotki always runs SyncRWLive sessions (chotki.go, *) +(* Open()); mode negotiation is therefore not modelled. *) +(* *) +(* Timers. Ping/pong timers, cleanSyncs() expiry and handshake timeouts *) +(* are modelled as nondeterministic action enablement (a timer that "may *) +(* fire"). *) +(* *) +(* ------------------------------ BUGS ---------------------------------- *) +(* *) +(* Switchable defect flags model defects found in the Go code while *) +(* writing this spec (see tla/README.md for TLC configs demonstrating *) +(* them, plus the non-modelled fixes that came out of the same work). *) +(* The cho.last flag is a local allocator witness added on top of the *) +(* original sync-protocol model. *) +(* *) +(* BuggyRelay = TRUE models replication/sync.go Drain() before the fix: *) +(* a drained batch whose last record is 'B' (bye) is NOT broadcast *) +(* at all, and a batch that fails mid-way relays nothing even though *) +(* a prefix was locally applied. Downstream replicas permanently *) +(* miss the applied records (violates NoGaps / QuiescentConverged). *) +(* BuggyRelay = FALSE models the fixed behaviour: relay exactly the *) +(* locally-applied prefix, minus the session-scoped trailing 'B'. *) +(* *) +(* DisplaceBySrc = FALSE models chotki.go drain() 'H' before the fix: *) +(* the "allow only 1 diff sync per src" cleanup compared sync-point *) +(* keys against cho.src (self), which never matches, so stale sync *) +(* points from an origin's previous session are never displaced *) +(* (violates SyncPointPerSrc). *) +(* DisplaceBySrc = TRUE models the fixed cleanup (compare against the *) +(* incoming handshake's source). *) +(* *) +(* DropSyncsOnClose = FALSE models cho.syncs before the fix: a sync *) +(* point (pending diff batch) survives the session whose 'H' created *) +(* it, waiting only for the cleanSyncs timeout. TLC finds (this bug *) +(* was discovered BY this model): a relayed 'H' creates a sync point *) +(* downstream; the relayed 'D' records die in the queue/channel of a *) +(* reconnecting downstream link; the relaying replica then feeds the *) +(* relayed 'V' into the NEW connection, which applies the staged *) +(* handshake VV without the data — permanent silent data loss *) +(* (violates NoGaps). *) +(* DropSyncsOnClose = TRUE models the fix: sync points are bound to *) +(* the session that created them (Syncer.SessionId / AbortSyncsVia) *) +(* and are aborted when it ends. *) +(* *) +(* ProtectLast = FALSE, together with EnableOwnSourceDrain = TRUE, *) +(* models cho.last as an unsynchronized allocator cache. If a local *) +(* drain applies a record stamped with this replica's own src but the *) +(* cho.last update is not visible to the next CommitPacket, that *) +(* commit can reuse an already issued seq (violates FreshLocalIds). *) +(* ProtectLast = TRUE models the intended fix: own-source drains and *) +(* local commits update/read cho.last under a synchronization edge. *) +(* *) +(* BatchMode = TRUE models chotki.go drain() applying the Y/C/O/E *) +(* records of a batch as one all-or-nothing pebble batch after the *) +(* loop (one write per drain) instead of per-packet. With *) +(* Both settings DROP the whole batch on a mid-batch error (current *) +(* drop-on-error drain: no partial prefix is ever persisted; the VV *) +(* bumps are dropped with the records). They differ only in the *) +(* cho.last advance. BatchBuggy = TRUE models the defect: cho.last is *) +(* advanced EAGERLY to the batch's own-source ids even when the batch *) +(* was dropped, leaving cho.last ahead of the persisted VV (violates *) +(* LastNotAhead). *) +(* BatchBuggy = FALSE models the fix: cho.last advances LAZILY, only *) +(* after the durable write, so a dropped batch consumes no id and *) +(* cho.last moves in lock-step with the version vector. *) +(***************************************************************************) + +EXTENDS Naturals, Sequences, FiniteSets + +CONSTANTS + Replicas, \* set of replica ids, e.g. {"a", "b", "c"} + Edges, \* tree topology: set of unordered pairs {x, y} + Objects, \* set of CRDT objects; 1 object == 1 sync block + CommitBudget, \* [Replicas -> Nat]: how many ops each replica commits + GenBudget, \* max number of sessions (connects) per edge + PingBudget, \* max pings initiated per endpoint per session + BuggyRelay, \* TRUE: model the pre-fix broadcast-relay behaviour + DisplaceBySrc, \* TRUE: model the fixed sync-point displacement + DropSyncsOnClose, \* TRUE: sync points die with the session (fixed) + ProtectLast, \* TRUE: cho.last is synchronized with own-source drains + EnableOwnSourceDrain, \* include the local cho.last race witness action + EnablePing, \* include the ping/pong keep-alive machinery + BatchMode, \* TRUE: model chotki.go drain() as one all-or-nothing + \* pebble batch for the Y/C/O/E (live) records, applied + \* after the loop, rather than per-packet. cho.last is + \* advanced to the batch's own-source ids only once the + \* batch is durable. + BatchBuggy \* only meaningful with BatchMode. Both drop the whole + \* batch on a mid-batch error (drop-on-error); this + \* toggles the cho.last advance. TRUE: EAGER — cho.last + \* advances to the batch's own-source ids even when the + \* batch was dropped (last runs past the VV). FALSE: the + \* fix — LAZY, cho.last advances only after the durable + \* write, so a dropped batch consumes no id. + +ASSUME + /\ \A p \in Edges : p \subseteq Replicas /\ Cardinality(p) = 2 + /\ CommitBudget \in [Replicas -> Nat] + /\ GenBudget \in Nat + /\ PingBudget \in Nat + /\ BuggyRelay \in BOOLEAN + /\ DisplaceBySrc \in BOOLEAN + /\ DropSyncsOnClose \in BOOLEAN + /\ ProtectLast \in BOOLEAN + /\ EnableOwnSourceDrain \in BOOLEAN + /\ EnablePing \in BOOLEAN + /\ BatchMode \in BOOLEAN + /\ BatchBuggy \in BOOLEAN + +(***************************************************************************) +(* An endpoint <> is the Syncer living at replica x serving the *) +(* connection to replica y. Its outbound records go to chan[<>]; *) +(* it drains chan[<>]. *) +(***************************************************************************) +Endpoints == {e \in Replicas \X Replicas : {e[1], e[2]} \in Edges} +Rev(e) == <> +EdgeOf(e) == {e[1], e[2]} + +Max(a, b) == IF a >= b THEN a ELSE b + +\* Largest element of a finite set of naturals (0 for the empty set). +SetMax(S) == IF S = {} THEN 0 ELSE CHOOSE m \in S : \A y \in S : y <= m + +ZeroVV == [s \in Replicas |-> 0] +VVMerge(u, w) == [s \in Replicas |-> Max(u[s], w[s])] + +(***************************************************************************) +(* Records (protocol packets). *) +(* H : handshake -- H(T{snaplast} M{mode} V{vv} S{trace}) *) +(* D : diff parcel -- D(T{snaplast} R{block} F+{ops}) *) +(* V : diff-final VV -- V(T{snaplast} (V(R{block} vv))* ) *) +(* B : bye -- B(T{snaplast} reason) *) +(* E : any mutation packet applied/relayed during live sync *) +(* P : ping/pong *) +(* A session key k == <> abstracts snaplast (the origin's own *) +(* last id inside its snapshot): FeedHandshake sets *) +(* snaplast = hostvv.GetID(sync.Src). *) +(***************************************************************************) +HRec(k, vv) == [t |-> "H", k |-> k, vv |-> vv] +DRec(k, o, ops) == [t |-> "D", k |-> k, o |-> o, ops |-> ops] +VRec(k, bv) == [t |-> "V", k |-> k, bv |-> bv] +BRec(k) == [t |-> "B", k |-> k] +ERec(op) == [t |-> "E", op |-> op] +PingRec == [t |-> "P", v |-> "ping"] +PongRec == [t |-> "P", v |-> "pong"] + +VARIABLES + \* ------------------------- replica (DB) state ---------------------- + store, \* [Replicas -> SUBSET Op] applied operations + gvv, \* [Replicas -> VV] global VV (VKey0) + bvv, \* [Replicas -> [Objects -> VV]] per-block VVs + last, \* [Replicas -> Nat] cached cho.last seq + issued, \* [Replicas -> SUBSET Nat] seqs stamped by this src + commitcnt, \* [Replicas -> Nat] local own-source events + syncs, \* [Replicas -> SUBSET SyncPoint] cho.syncs: pending + \* diff batches; sp = [k, hvv, ops]; sp.hvv is the origin's + \* handshake VV staged in the batch (ApplyH), applied + \* atomically together with sp.ops when 'V' arrives. + \* ------------------------- per-endpoint state ---------------------- + feed, \* [Endpoints -> {"hs","diff","live","eof","none"}] + drain, \* [Endpoints -> {"hs","diff","live","none"}] + outq, \* [Endpoints -> Seq(Record)] broadcast queue (FDQueue) + chan, \* [Endpoints -> Seq(Record)] records in flight e[1] -> e[2] + peervv, \* [Endpoints -> VV] peer's handshake VV + snap, \* [Endpoints -> [key, bvv, ops, pend, vset]] pebble snapshot + \* taken by FeedHandshake; pend = blocks not yet examined, + \* vset = blocks with changes (they go into the 'V' packet) + ping, \* [Endpoints -> {"idle","wait_pong","want_pong"}] + pingcnt, \* [Endpoints -> Nat] pings initiated (bounding only) + gen \* [Edges -> Nat] sessions started on this edge so far + +vars == <> + +NoSnap(x) == [key |-> <>, + bvv |-> [o \in Objects |-> ZeroVV], + ops |-> {}, + pend |-> {}, + vset |-> {}] + +(***************************************************************************) +(* Type invariant. *) +(***************************************************************************) +IsVV(v) == v \in [Replicas -> Nat] +IsOp(op) == /\ DOMAIN op = {"src", "seq", "obj"} + /\ op.src \in Replicas /\ op.seq \in Nat \ {0} /\ op.obj \in Objects +IsKey(k) == k \in Replicas \X Nat + +IsRec(m) == + \/ /\ m.t = "H" /\ DOMAIN m = {"t", "k", "vv"} + /\ IsKey(m.k) /\ IsVV(m.vv) + \/ /\ m.t = "D" /\ DOMAIN m = {"t", "k", "o", "ops"} + /\ IsKey(m.k) /\ m.o \in Objects /\ \A op \in m.ops : IsOp(op) + \/ /\ m.t = "V" /\ DOMAIN m = {"t", "k", "bv"} + /\ IsKey(m.k) /\ DOMAIN m.bv \subseteq Objects + /\ \A o \in DOMAIN m.bv : IsVV(m.bv[o]) + \/ /\ m.t = "B" /\ DOMAIN m = {"t", "k"} /\ IsKey(m.k) + \/ /\ m.t = "E" /\ DOMAIN m = {"t", "op"} /\ IsOp(m.op) + \/ /\ m.t = "P" /\ DOMAIN m = {"t", "v"} /\ m.v \in {"ping", "pong"} + +IsSyncPoint(sp) == /\ DOMAIN sp = {"k", "hvv", "ops", "via"} + /\ IsKey(sp.k) /\ IsVV(sp.hvv) /\ \A op \in sp.ops : IsOp(op) + /\ sp.via \in Endpoints + +TypeOK == + /\ \A r \in Replicas : + /\ \A op \in store[r] : IsOp(op) + /\ IsVV(gvv[r]) + /\ \A o \in Objects : IsVV(bvv[r][o]) + /\ last[r] \in Nat + /\ issued[r] \subseteq (Nat \ {0}) + /\ commitcnt[r] \in Nat + /\ \A sp \in syncs[r] : IsSyncPoint(sp) + /\ \A e \in Endpoints : + /\ feed[e] \in {"hs", "diff", "live", "eof", "none"} + /\ drain[e] \in {"hs", "diff", "live", "none"} + /\ \A i \in 1..Len(outq[e]) : IsRec(outq[e][i]) + /\ \A i \in 1..Len(chan[e]) : IsRec(chan[e][i]) + /\ IsVV(peervv[e]) + /\ IsKey(snap[e].key) + /\ snap[e].pend \subseteq Objects /\ snap[e].vset \subseteq Objects + /\ ping[e] \in {"idle", "wait_pong", "want_pong"} + /\ pingcnt[e] \in Nat + /\ \A p \in Edges : gen[p] \in Nat + +(***************************************************************************) +(* Broadcast (chotki.go Broadcast): append records to the outbound queue *) +(* of every other live session at replica x ("except" semantics). A *) +(* queue exists for the lifetime of the connection. *) +(***************************************************************************) +QueueOpen(d) == feed[d] # "none" /\ drain[d] # "none" + +BcastTargets(x, except) == + {d \in Endpoints : d[1] = x /\ d # except /\ QueueOpen(d)} + +SeqToApp(q, recs) == q \o recs + +(***************************************************************************) +(* Chotki.drain applied record-by-record over a batch (chotki.go). *) +(* The fold state st carries the replica's DB view plus: *) +(* err : a record failed (ErrSyncUnknown & co) -- processing stops; *) +(* n : how many records of the batch were fully applied. *) +(***************************************************************************) +ApplyOne(st, m) == + CASE m.t = "H" -> + \* chotki.go 'H': displace stale sync points, then create a + \* fresh one keyed by the handshake id (snaplast). ApplyH + \* stages the origin's global VV inside the batch. The sync + \* point remembers the session (endpoint) that delivered the + \* handshake (syncPoint.via). + LET kept == IF DisplaceBySrc + THEN {sp \in st.syn : sp.k[1] # m.k[1]} + ELSE {sp \in st.syn : sp.k # m.k} + \* pre-fix code: cho.syncs.Store overwrites the + \* same key; same-src keys are NOT displaced + IN [st EXCEPT !.syn = kept \cup + {[k |-> m.k, hvv |-> m.vv, ops |-> {}, via |-> st.via]}] + [] m.t = "D" -> + \* chotki.go 'D': stage ops into the sync-point batch; + \* unknown sync point => ErrSyncUnknown. + IF \E sp \in st.syn : sp.k = m.k + THEN LET sp == CHOOSE sp \in st.syn : sp.k = m.k + IN [st EXCEPT !.syn = (@ \ {sp}) \cup + {[sp EXCEPT !.ops = @ \cup m.ops]}] + ELSE [st EXCEPT !.err = TRUE] + [] m.t = "V" -> + \* chotki.go 'V': atomically apply the whole batch (staged data + \* ops + staged handshake VV) and the block VVs carried by 'V'. + IF \E sp \in st.syn : sp.k = m.k + THEN LET sp == CHOOSE sp \in st.syn : sp.k = m.k + IN [st EXCEPT + !.sto = @ \cup sp.ops, + !.gv = VVMerge(@, sp.hvv), + !.bv = [o \in Objects |-> + IF o \in DOMAIN m.bv + THEN VVMerge(st.bv[o], m.bv[o]) + ELSE st.bv[o]], + !.syn = @ \ {sp}] + ELSE [st EXCEPT !.err = TRUE] + [] m.t = "B" -> + \* chotki.go 'B': drop the origin's pending diff batch, if any. + [st EXCEPT !.syn = {sp \in @ : sp.k # m.k}] + [] m.t = "E" -> + \* chotki.go 'E'/'O'/...: a live mutation. In BatchMode it is only + \* staged into the pending batch (pe), applied all-or-nothing after + \* the loop; the global/block VV bumps and the cho.last advance + \* happen at that commit, not here (see the Drain action). Without + \* BatchMode it is applied immediately (the old per-packet drain): + \* UpdateVTree bumps both the global VV and the object's block VV. + IF BatchMode + THEN [st EXCEPT !.pe = @ \cup {m.op}] + ELSE [st EXCEPT !.sto = @ \cup {m.op}, + !.gv = [@ EXCEPT ![m.op.src] = Max(@, m.op.seq)], + !.bv = [@ EXCEPT ![m.op.obj][m.op.src] = Max(@, m.op.seq)], + !.lst = IF ProtectLast /\ m.op.src = st.self + THEN Max(@, m.op.seq) + ELSE @] + +RECURSIVE ApplyBatch(_, _) +ApplyBatch(st, recs) == + IF recs = <<>> \/ st.err THEN st + ELSE LET st1 == ApplyOne(st, Head(recs)) + IN ApplyBatch(IF st1.err THEN st1 ELSE [st1 EXCEPT !.n = @ + 1], + Tail(recs)) + +(***************************************************************************) +(* Ping records are removed by Syncer.processPings before the batch *) +(* reaches Chotki.drain and are never relayed. *) +(***************************************************************************) +RECURSIVE FilterP(_) +FilterP(s) == + IF s = <<>> THEN <<>> + ELSE IF Head(s).t = "P" THEN FilterP(Tail(s)) + ELSE <> \o FilterP(Tail(s)) + +HasPing(s) == \E i \in 1..Len(s) : s[i].t = "P" /\ s[i].v = "ping" + +(***************************************************************************) +(* What Syncer.Drain rebroadcasts to the other sessions. *) +(* *) +(* BuggyRelay = TRUE (replication/sync.go before the fix): *) +(* - the handshake head is always relayed once accepted; *) +(* - the rest of the batch is relayed only if the whole batch applied *) +(* cleanly AND its last record is not 'B'. *) +(* BuggyRelay = FALSE (fixed): *) +(* - relay exactly the locally-applied prefix, minus a trailing 'B' *) +(* (a bye is session-scoped and must not leak downstream). *) +(***************************************************************************) +RelayOf(batch, st, wasHs) == + IF BuggyRelay + THEN LET rest == IF wasHs THEN Tail(batch) ELSE batch + headRelay == IF wasHs /\ st.n >= 1 THEN <> ELSE <<>> + tailB == rest # <<>> /\ rest[Len(rest)].t = "B" + IN IF st.err \/ tailB THEN headRelay ELSE headRelay \o rest + ELSE IF st.err /\ BatchMode + \* Drop-on-error: the batch was dropped, so drain returns applied=0 and + \* relayApplied broadcasts NOTHING. Relaying the applied prefix here + \* would advertise records this replica did not persist. + THEN <<>> + ELSE LET pre == SubSeq(batch, 1, st.n) + IN IF pre # <<>> /\ pre[Len(pre)].t = "B" + THEN SubSeq(pre, 1, Len(pre) - 1) + ELSE pre + +(***************************************************************************) +(* Syncer.Drain state transition, driven by the last record of the batch *) +(* (LastLit). *) +(***************************************************************************) +NextDrainState(cur, batch) == + LET lastT == batch[Len(batch)].t + IN CASE cur = "hs" -> + IF Len(batch) = 1 THEN "diff" \* just the handshake + ELSE IF lastT \in {"D", "V"} THEN "diff" + ELSE IF lastT = "B" THEN "none" ELSE "live" + [] cur = "diff" -> + IF lastT \in {"D", "V"} THEN "diff" + ELSE IF lastT = "B" THEN "none" ELSE "live" + [] cur = "live" -> + IF lastT = "B" THEN "none" ELSE "live" + +-------------------------------------------------------------------------- + +Init == + /\ store = [r \in Replicas |-> {}] + /\ gvv = [r \in Replicas |-> ZeroVV] + /\ bvv = [r \in Replicas |-> [o \in Objects |-> ZeroVV]] + /\ last = [r \in Replicas |-> 0] + /\ issued = [r \in Replicas |-> {}] + /\ commitcnt = [r \in Replicas |-> 0] + /\ syncs = [r \in Replicas |-> {}] + /\ feed = [e \in Endpoints |-> "none"] + /\ drain = [e \in Endpoints |-> "none"] + /\ outq = [e \in Endpoints |-> <<>>] + /\ chan = [e \in Endpoints |-> <<>>] + /\ peervv = [e \in Endpoints |-> ZeroVV] + /\ snap = [e \in Endpoints |-> NoSnap(e[1])] + /\ ping = [e \in Endpoints |-> "idle"] + /\ pingcnt = [e \in Endpoints |-> 0] + /\ gen = [p \in Edges |-> 0] + +(***************************************************************************) +(* Connect / reconnect an edge (network.Net dial + accept). Both *) +(* endpoints start a fresh Syncer. Anything still in flight or queued *) +(* from the previous session is dropped with the old connection. Sync *) +(* points at either replica survive: they live in cho.syncs, not in the *) +(* Syncer. *) +(***************************************************************************) +\* Sync points are bound to the session that created them and die with it +\* (Syncer.Close -> AbortSyncsVia); pre-fix they lingered until the +\* cleanSyncs timeout. +DropVia(S, p) == IF DropSyncsOnClose + THEN {sp \in S : EdgeOf(sp.via) # p} + ELSE S + +Connect(p) == + /\ gen[p] < GenBudget + /\ \A e \in Endpoints : EdgeOf(e) = p => feed[e] = "none" + /\ syncs' = [r \in Replicas |-> + IF r \in p THEN DropVia(syncs[r], p) ELSE syncs[r]] + /\ gen' = [gen EXCEPT ![p] = @ + 1] + /\ feed' = [e \in Endpoints |-> IF EdgeOf(e) = p THEN "hs" ELSE feed[e]] + /\ drain' = [e \in Endpoints |-> IF EdgeOf(e) = p THEN "hs" ELSE drain[e]] + /\ chan' = [e \in Endpoints |-> IF EdgeOf(e) = p THEN <<>> ELSE chan[e]] + /\ outq' = [e \in Endpoints |-> IF EdgeOf(e) = p THEN <<>> ELSE outq[e]] + /\ peervv' = [e \in Endpoints |-> IF EdgeOf(e) = p THEN ZeroVV ELSE peervv[e]] + /\ snap' = [e \in Endpoints |-> IF EdgeOf(e) = p THEN NoSnap(e[1]) ELSE snap[e]] + /\ ping' = [e \in Endpoints |-> IF EdgeOf(e) = p THEN "idle" ELSE ping[e]] + /\ pingcnt' = [e \in Endpoints |-> IF EdgeOf(e) = p THEN 0 ELSE pingcnt[e]] + /\ UNCHANGED <> + +(***************************************************************************) +(* CommitPacket (chotki.go): a replica commits a local op, applies it to *) +(* its own DB and broadcasts it to every live session (except ""). *) +(***************************************************************************) +Commit(r, o) == + /\ commitcnt[r] < CommitBudget[r] + /\ LET q == last[r] + 1 + op == [src |-> r, seq |-> q, obj |-> o] + IN /\ store' = [store EXCEPT ![r] = @ \cup {op}] + /\ gvv' = [gvv EXCEPT ![r][r] = Max(@, q)] + /\ bvv' = [bvv EXCEPT ![r][o][r] = Max(@, q)] + /\ last' = [last EXCEPT ![r] = q] + /\ issued' = [issued EXCEPT ![r] = @ \cup {q}] + /\ commitcnt' = [commitcnt EXCEPT ![r] = @ + 1] + /\ outq' = [d \in Endpoints |-> + IF d \in BcastTargets(r, <>) + THEN SeqToApp(outq[d], <>) + ELSE outq[d]] + /\ UNCHANGED <> + +\* chotki.drain can also consume records stamped with this replica's own +\* src (for example replay/import or an old process using the same source). +\* The Go code tried to advance cho.last on that path, but without a +\* separate synchronization edge the next CommitPacket may observe stale +\* cho.last and reuse a seq. ProtectLast=FALSE models the lost visibility. +\* +\* In BatchMode this record rides a batched drain that then errors. Drop-on-error +\* (current chotki.go): the whole batch is dropped, so its VV bump is lost +\* (store/gvv/bvv unchanged). BatchBuggy TRUE models the defect where cho.last was +\* advanced EAGERLY to its id even though the batch was dropped, leaving cho.last +\* ahead of the persisted VV (LastNotAhead) and reissuing the seq. BatchBuggy +\* FALSE is the fix: cho.last advances only alongside the durable write, so a +\* dropped batch consumes no id and last moves in lock-step with the VV. +OwnSourceDrain(r, o) == + /\ EnableOwnSourceDrain + /\ commitcnt[r] < CommitBudget[r] + /\ LET q == gvv[r][r] + 1 + op == [src |-> r, seq |-> q, obj |-> o] + \* drop-on-error: a batched drain that errors persists nothing + dropped == BatchMode + \* the fix advances cho.last only with the durable write (a dropped + \* batch consumes no id); the bug advances it eagerly even on a drop + advanceLast == ~dropped \/ BatchBuggy + IN /\ store' = IF dropped THEN store ELSE [store EXCEPT ![r] = @ \cup {op}] + /\ gvv' = IF dropped THEN gvv ELSE [gvv EXCEPT ![r][r] = q] + /\ bvv' = IF dropped THEN bvv ELSE [bvv EXCEPT ![r][o][r] = q] + /\ last' = IF ProtectLast /\ advanceLast + THEN [last EXCEPT ![r] = Max(@, q)] + ELSE last + \* the id is consumed (issued, counted) only when cho.last advances to + \* it; a dropped-and-not-advanced batch is a no-op + /\ issued' = IF advanceLast THEN [issued EXCEPT ![r] = @ \cup {q}] ELSE issued + /\ commitcnt' = IF advanceLast THEN [commitcnt EXCEPT ![r] = @ + 1] ELSE commitcnt + /\ UNCHANGED <> + +(***************************************************************************) +(* Feed side (replication/sync.go Feed()). *) +(***************************************************************************) + +\* FeedHandshake: take a pebble snapshot, send H(snaplast, mode, vv). +FeedHandshake(e) == + /\ feed[e] = "hs" + /\ drain[e] # "none" + /\ LET x == e[1] + k == <> + IN /\ chan' = [chan EXCEPT ![e] = Append(@, HRec(k, gvv[x]))] + /\ snap' = [snap EXCEPT ![e] = [key |-> k, + bvv |-> bvv[x], + ops |-> store[x], + pend |-> Objects, + vset |-> {}]] + /\ feed' = [feed EXCEPT ![e] = "diff"] + /\ UNCHANGED <> + +\* FeedBlockDiff: examine the next block of the snapshot. Gated on the +\* peer's handshake having been drained (WaitDrainState(SendDiff)). A +\* block is sent iff its block VV is ahead of the peer's global VV +\* (getVVChanges); the parcel carries the ops the peer lacks. +FeedBlockDiff(e) == + /\ feed[e] = "diff" + /\ drain[e] \in {"diff", "live"} + /\ snap[e].pend # {} + /\ \E o \in snap[e].pend : + LET hasChanges == \E s \in Replicas : snap[e].bvv[o][s] > peervv[e][s] + ops == {op \in snap[e].ops : + op.obj = o /\ op.seq > peervv[e][op.src]} + IN IF hasChanges + THEN /\ chan' = [chan EXCEPT ![e] = Append(@, DRec(snap[e].key, o, ops))] + /\ snap' = [snap EXCEPT ![e].pend = @ \ {o}, + ![e].vset = @ \cup {o}] + ELSE /\ chan' = chan + /\ snap' = [snap EXCEPT ![e].pend = @ \ {o}] + /\ UNCHANGED <> + +\* FeedDiffVV: all blocks examined; send the 'V' packet carrying the +\* snapshot block VVs of every changed block, then go live (SyncLive is +\* always set, so the non-live SendEOF branch is not taken). +FeedDiffVV(e) == + /\ feed[e] = "diff" + /\ drain[e] \in {"diff", "live"} + /\ snap[e].pend = {} + /\ chan' = [chan EXCEPT ![e] = + Append(@, VRec(snap[e].key, + [o \in snap[e].vset |-> snap[e].bvv[o]]))] + /\ feed' = [feed EXCEPT ![e] = "live"] + /\ UNCHANGED <> + +\* FeedLive: ship the accumulated broadcast queue (Oqueue.Feed). +FeedLive(e) == + /\ feed[e] = "live" + /\ drain[e] \in {"diff", "live"} + /\ outq[e] # <<>> + /\ chan' = [chan EXCEPT ![e] = @ \o outq[e]] + /\ outq' = [outq EXCEPT ![e] = <<>>] + /\ UNCHANGED <> + +\* The session's outbound queue is closed (displacement by a new +\* connection, queue overflow, shutdown) or the ping timeout fired: +\* Feed switches to SendEOF. Bounded by the reconnect budget so that +\* TLC does not have to explore pointless final disconnects. +FeedClose(e) == + /\ feed[e] = "live" + /\ outq[e] = <<>> + /\ gen[EdgeOf(e)] < GenBudget + /\ feed' = [feed EXCEPT ![e] = "eof"] + /\ UNCHANGED <> + +\* SendEOF: emit B(snaplast, reason) and stop feeding. +FeedEOF(e) == + /\ feed[e] = "eof" + /\ drain[e] # "none" + /\ chan' = [chan EXCEPT ![e] = Append(@, BRec(snap[e].key))] + /\ feed' = [feed EXCEPT ![e] = "none"] + /\ UNCHANGED <> + +\* Feed() checks GetDrainState() == SendNone on every call and winds the +\* feed down -- but only once its own handshake/diff phase is complete: a +\* peer's bye means it has nothing more to send, while its drain side +\* keeps applying our records until the connection closes, so cutting our +\* diff short would strand a staged diff batch at the peer without its +\* 'V' (the peer would silently miss the promised data). +FeedDrainNone(e) == + /\ drain[e] = "none" + /\ feed[e] \in {"live", "eof"} + /\ feed' = [feed EXCEPT ![e] = "none"] + /\ UNCHANGED <> + +(***************************************************************************) +(* Ping/pong (replication/sync.go pingTransition / processPings). *) +(***************************************************************************) +FeedPing(e) == + /\ EnablePing + /\ feed[e] = "live" /\ ping[e] = "idle" /\ pingcnt[e] < PingBudget + /\ chan' = [chan EXCEPT ![e] = Append(@, PingRec)] + /\ ping' = [ping EXCEPT ![e] = "wait_pong"] + /\ pingcnt' = [pingcnt EXCEPT ![e] = @ + 1] + /\ UNCHANGED <> + +FeedPong(e) == + /\ EnablePing + /\ feed[e] = "live" /\ ping[e] = "want_pong" + /\ chan' = [chan EXCEPT ![e] = Append(@, PongRec)] + /\ ping' = [ping EXCEPT ![e] = "idle"] + /\ UNCHANGED <> + +\* The PingWait timer fires with no pong (PingBroken): the session ends. +PingBroken(e) == + /\ EnablePing + /\ feed[e] = "live" /\ ping[e] = "wait_pong" + /\ gen[EdgeOf(e)] < GenBudget + /\ feed' = [feed EXCEPT ![e] = "eof"] + /\ UNCHANGED <> + +(***************************************************************************) +(* Drain side (replication/sync.go Drain + chotki.go drain). *) +(* *) +(* One Drain call processes a coalesced batch: an arbitrary non-empty *) +(* prefix of the incoming channel (network/peer.go keepRead). *) +(***************************************************************************) + +\* An unrecoverable drain error (bad handshake, ErrSyncUnknown, ...): +\* Drain returns the error, the network layer tears the connection down, +\* and everything in flight or queued dies with it, including the sync +\* points this session had created (syn0 is the [Replicas -> syncs] base +\* to start from, so the caller can fold in partial batch application). +KillEdge(e, syn0) == + LET p == EdgeOf(e) + IN /\ feed' = [d \in Endpoints |-> IF EdgeOf(d) = p THEN "none" ELSE feed[d]] + /\ drain' = [d \in Endpoints |-> IF EdgeOf(d) = p THEN "none" ELSE drain[d]] + /\ chan' = [d \in Endpoints |-> IF EdgeOf(d) = p THEN <<>> ELSE chan[d]] + /\ syncs' = [r \in Replicas |-> + IF r \in p THEN DropVia(syn0[r], p) ELSE syn0[r]] + +Drain(e) == + /\ drain[e] \in {"hs", "diff", "live"} + /\ chan[Rev(e)] # <<>> + /\ \E n \in 1..Len(chan[Rev(e)]) : + LET x == e[1] + raw == SubSeq(chan[Rev(e)], 1, n) + rest == SubSeq(chan[Rev(e)], n + 1, Len(chan[Rev(e)])) + batch == FilterP(raw) + \* processPings: a ping means we owe a pong; any traffic + \* resets a pending WaitingForPing (resetPingTimer). + ping1 == IF HasPing(raw) THEN "want_pong" + ELSE IF ping[e] = "wait_pong" THEN "idle" ELSE ping[e] + badHs == drain[e] = "hs" /\ (batch = <<>> \/ batch[1].t # "H") + IN + IF badHs + THEN \* ErrBadHPacket: session dies. + /\ KillEdge(e, syncs) + /\ ping' = [ping EXCEPT ![e] = ping1] + /\ UNCHANGED <> + ELSE IF batch = <<>> + THEN \* batch was pings only: nothing reaches Chotki.drain. + /\ chan' = [chan EXCEPT ![Rev(e)] = rest] + /\ ping' = [ping EXCEPT ![e] = ping1] + /\ UNCHANGED <> + ELSE + LET st0 == [sto |-> store[x], gv |-> gvv[x], bv |-> bvv[x], + syn |-> syncs[x], via |-> e, self |-> x, + lst |-> last[x], err |-> FALSE, n |-> 0, pe |-> {}] + stF == ApplyBatch(st0, batch) + rly == RelayOf(batch, stF, drain[e] = "hs") + tgt == BcastTargets(x, e) + nds == NextDrainState(drain[e], batch) + \* Commit the pending batch (pe) accumulated in BatchMode. + \* Drop-on-error (current chotki.go drain): a mid-batch error + \* discards the WHOLE pending batch — no partial prefix is ever + \* made durable (keepPE FALSE on error, for both the fix and the + \* bug). Outside BatchMode pe is empty, so all of this is a no-op + \* and the per-packet fold result (stF) is committed unchanged. + keepPE == ~stF.err + peGv == [s \in Replicas |-> + Max(stF.gv[s], + SetMax({op.seq : op \in {o \in stF.pe : o.src = s}}))] + peBv == [o \in Objects |-> [s \in Replicas |-> + Max(stF.bv[o][s], + SetMax({op.seq : op \in {p \in stF.pe : + p.obj = o /\ p.src = s}}))]] + ownMax == SetMax({op.seq : op \in {o \in stF.pe : o.src = x}}) + finalSto == IF keepPE THEN stF.sto \cup stF.pe ELSE stF.sto + finalGv == IF keepPE THEN peGv ELSE stF.gv + finalBv == IF keepPE THEN peBv ELSE stF.bv + \* cho.last advance to the batch's own-source ids. The fix + \* (BatchBuggy FALSE) is LAZY: it advances only after the durable + \* write (gated on keepPE), so last stays == VV. The bug + \* (BatchBuggy TRUE) is EAGER: it advances even when the batch was + \* dropped on error (keepPE FALSE), so last runs past the VV + \* (violates LastNotAhead). + finalLst == IF ProtectLast /\ (keepPE \/ BatchBuggy) + THEN Max(stF.lst, ownMax) ELSE stF.lst + IN /\ store' = [store EXCEPT ![x] = finalSto] + /\ gvv' = [gvv EXCEPT ![x] = finalGv] + /\ bvv' = [bvv EXCEPT ![x] = finalBv] + /\ last' = [last EXCEPT ![x] = finalLst] + /\ outq' = [d \in Endpoints |-> + IF d \in tgt /\ rly # <<>> + THEN SeqToApp(outq[d], rly) + ELSE outq[d]] + /\ peervv' = IF drain[e] = "hs" + THEN [peervv EXCEPT ![e] = batch[1].vv] + ELSE peervv + /\ ping' = [ping EXCEPT ![e] = ping1] + /\ IF stF.err + THEN \* Drain returns the error; the connection dies + KillEdge(e, [syncs EXCEPT ![x] = stF.syn]) + ELSE /\ syncs' = [syncs EXCEPT ![x] = stF.syn] + /\ drain' = [drain EXCEPT ![e] = nds] + /\ chan' = [chan EXCEPT ![Rev(e)] = rest] + /\ feed' = feed + /\ UNCHANGED <> + +-------------------------------------------------------------------------- + +Next == + \/ \E p \in Edges : Connect(p) + \/ \E r \in Replicas, o \in Objects : Commit(r, o) + \/ \E r \in Replicas, o \in Objects : OwnSourceDrain(r, o) + \/ \E e \in Endpoints : + \/ FeedHandshake(e) \/ FeedBlockDiff(e) \/ FeedDiffVV(e) + \/ FeedLive(e) \/ FeedClose(e) \/ FeedEOF(e) \/ FeedDrainNone(e) + \/ FeedPing(e) \/ FeedPong(e) \/ PingBroken(e) + \/ Drain(e) + +Spec == Init /\ [][Next]_vars + +-------------------------------------------------------------------------- +(***************************************************************************) +(* Safety properties. *) +(***************************************************************************) + +\* Every op ever committed by replica s with seq q (ops are committed with +\* consecutive seqs, so "q <= gvv[s][s]" iff it was committed). +Committed(s, q) == q <= gvv[s][s] + +(***************************************************************************) +(* NoGaps -- the central data-safety property of the protocol. A *) +(* replica's global VV must never claim knowledge of an op it does not *) +(* actually store: diff sync uses the peer's global VV as the resend *) +(* floor (getVVChanges/sendvv), so an op below that floor which is not *) +(* in the store will never be re-sent by anyone -- permanent data loss. *) +(***************************************************************************) +NoGaps == + \A r \in Replicas, s \in Replicas : + \A q \in 1..gvv[r][s] : + \E op \in store[r] : op.src = s /\ op.seq = q + +\* A replica never claims to have seen more from s than s ever committed. +VVBounded == + \A r \in Replicas, s \in Replicas : gvv[r][s] <= gvv[s][s] + +\* cho.last is the cached allocator state used by CommitPacket to stamp +\* the next local id. It must never lag behind the replica's own VV entry. +LastCoversOwnVV == + \A r \in Replicas : last[r] >= gvv[r][r] + +\* ...nor run AHEAD of the persisted own VV entry. cho.last lives only in +\* memory: a restart rebuilds it from the persisted version vector +\* (cho.last = vv.GetID(cho.src) in Open). If cho.last was advanced to an id +\* whose op the batched drain then dropped (never persisted to the VV), a +\* restart rebuilds cho.last behind that id and the next commit reissues it, +\* though a peer that received the drained/relayed record already holds it. +\* Together with LastCoversOwnVV this pins last[r] = gvv[r][r]: the allocator +\* must move in lock-step with the durable version vector. +LastNotAhead == + \A r \in Replicas : last[r] <= gvv[r][r] + +\* Every local own-source event must have received a fresh seq. issued is +\* a set, while commitcnt counts events; a repeated seq shrinks the set. +FreshLocalIds == + \A r \in Replicas : Cardinality(issued[r]) = commitcnt[r] + +\* Block VVs are exact: a non-zero bvv entry names a stored op stamp... +BvvExact == + \A r \in Replicas, o \in Objects, s \in Replicas : + bvv[r][o][s] = 0 \/ + \E op \in store[r] : + op.src = s /\ op.seq = bvv[r][o][s] /\ op.obj = o + +\* ...and complete: they cover every stored op, otherwise the sender-side +\* hasChanges check (block VV vs peer global VV) would under-send. +BvvCovers == + \A r \in Replicas : \A op \in store[r] : + op.seq <= bvv[r][op.obj][op.src] + +(***************************************************************************) +(* "Allow only 1 diff sync per src" (chotki.go drain 'H'): at any moment *) +(* a replica holds at most one pending diff batch per origin replica. *) +(***************************************************************************) +SyncPointPerSrc == + \A r \in Replicas : + \A sp1, sp2 \in syncs[r] : sp1.k[1] = sp2.k[1] => sp1 = sp2 + +(***************************************************************************) +(* Convergence. When nothing is in flight, no broadcast is queued and *) +(* every session has finished its diff phase (all feeds live), all *) +(* replicas of the (tree-shaped) cluster must hold the same data. *) +(***************************************************************************) +TransQuiescent == + \A e \in Endpoints : + chan[e] = <<>> /\ outq[e] = <<>> /\ feed[e] = "live" + +Converged == \A r1, r2 \in Replicas : store[r1] = store[r2] + +QuiescentConverged == TransQuiescent => Converged + +\* Used with "INVARIANT NotConverged" as a reachability witness: TLC's +\* counterexample proves full convergence is actually reachable. +NotConverged == + ~(Converged /\ TransQuiescent /\ \A r \in Replicas : gvv[r][r] = CommitBudget[r]) + +============================================================================= diff --git a/tla/MCBatchBuggy.cfg b/tla/MCBatchBuggy.cfg new file mode 100644 index 0000000..2a27f5a --- /dev/null +++ b/tla/MCBatchBuggy.cfg @@ -0,0 +1,28 @@ +\* The pre-fix batched drain: a mid-batch error drops the staged Y/C/O/E records +\* (their version-vector bumps dropped with them) yet cho.last was already +\* advanced to their own-source ids. cho.last is left ahead of the persisted VV; +\* a restart rebuilds it from the VV (behind) and the next commit reissues an id +\* a peer already holds. Same allocator model as MCBuggyLast, batched + buggy. +\* Expected: LastNotAhead VIOLATED (a tiny trace: one dropped own-source drain +\* advances last to 1 while the persisted VV stays 0). +SPECIFICATION Spec +CONSTANTS + Replicas <- MCReplicas2 + Edges <- MCEdges2 + Objects <- MCObjects + CommitBudget <- MCBudgetA2Last + GenBudget = 0 + PingBudget = 0 + BuggyRelay = FALSE + DisplaceBySrc = TRUE + DropSyncsOnClose = TRUE + ProtectLast = TRUE + EnableOwnSourceDrain = TRUE + EnablePing = FALSE + BatchMode = TRUE + BatchBuggy = TRUE +INVARIANTS + TypeOK + LastCoversOwnVV + LastNotAhead + FreshLocalIds diff --git a/tla/MCBatchFixed.cfg b/tla/MCBatchFixed.cfg new file mode 100644 index 0000000..a5640e3 --- /dev/null +++ b/tla/MCBatchFixed.cfg @@ -0,0 +1,34 @@ +\* The fixed batched drain: chotki.go drain() applies the Y/C/O/E records as one +\* all-or-nothing pebble batch (BatchMode) with drop-on-error semantics — on a +\* mid-batch error nothing is applied or relayed and cho.last does not advance; +\* on success cho.last advances only after the durable write. Same +\* three-replica chain as MCFixed, exercising the batched live/diff relay end to +\* end (the local id allocator under batching is covered by MCBatchFixedLast). +\* Expected: no violations (NoGaps, QuiescentConverged, LastNotAhead, ... hold). +SPECIFICATION Spec +CONSTANTS + Replicas <- MCReplicas + Edges <- MCEdgesLine + Objects <- MCObjects + CommitBudget <- MCBudgetAC + GenBudget = 1 + PingBudget = 0 + BuggyRelay = FALSE + DisplaceBySrc = TRUE + DropSyncsOnClose = TRUE + ProtectLast = TRUE + EnableOwnSourceDrain = FALSE + EnablePing = FALSE + BatchMode = TRUE + BatchBuggy = FALSE +INVARIANTS + TypeOK + NoGaps + VVBounded + LastCoversOwnVV + LastNotAhead + FreshLocalIds + BvvExact + BvvCovers + SyncPointPerSrc + QuiescentConverged diff --git a/tla/MCBatchFixedLast.cfg b/tla/MCBatchFixedLast.cfg new file mode 100644 index 0000000..212a8f3 --- /dev/null +++ b/tla/MCBatchFixedLast.cfg @@ -0,0 +1,35 @@ +\* The fixed batched drain, allocator focus: same own-source drain witness as +\* MCFixedLast, but with the batched all-or-nothing apply (BatchMode) and the +\* fix (BatchBuggy FALSE) — drop-on-error: an erroring batch persists nothing +\* and consumes no id, so cho.last must move in lock-step with the persisted +\* VV. NOTE: in BatchMode the own-source drain models only the DROPPED batch +\* (OwnSourceDrain hardcodes dropped == BatchMode), so the successful batched +\* own-source drain path is not explored here. (QuiescentConverged is not +\* checked: the own-source drain injects data it never broadcasts, so it +\* cannot converge — same as MCFixedLast.) +\* Expected: no violations (LastNotAhead, LastCoversOwnVV, FreshLocalIds hold). +SPECIFICATION Spec +CONSTANTS + Replicas <- MCReplicas2 + Edges <- MCEdges2 + Objects <- MCObjects + CommitBudget <- MCBudgetA2Last + GenBudget = 0 + PingBudget = 0 + BuggyRelay = FALSE + DisplaceBySrc = TRUE + DropSyncsOnClose = TRUE + ProtectLast = TRUE + EnableOwnSourceDrain = TRUE + EnablePing = FALSE + BatchMode = TRUE + BatchBuggy = FALSE +INVARIANTS + TypeOK + NoGaps + VVBounded + LastCoversOwnVV + LastNotAhead + FreshLocalIds + BvvExact + BvvCovers diff --git a/tla/MCBuggyDisplace.cfg b/tla/MCBuggyDisplace.cfg new file mode 100644 index 0000000..7cf7622 --- /dev/null +++ b/tla/MCBuggyDisplace.cfg @@ -0,0 +1,24 @@ +\* Demonstrates the sync-point displacement bug of chotki.go drain() 'H' +\* (fixed in this commit): the "allow only 1 diff sync per src" cleanup +\* compared sync-point keys against cho.src (the local replica) instead of +\* the incoming handshake's source, so it never displaced anything and a +\* replica could accumulate several pending diff batches for one origin. +\* Expected: SyncPointPerSrc VIOLATED. +SPECIFICATION Spec +CONSTANTS + Replicas <- MCReplicas + Edges <- MCEdgesLine + Objects <- MCObjects + CommitBudget <- MCBudgetA3 + GenBudget = 2 + PingBudget = 0 + BuggyRelay = FALSE + DisplaceBySrc = FALSE + DropSyncsOnClose = TRUE + ProtectLast = TRUE + EnableOwnSourceDrain = FALSE + EnablePing = FALSE + BatchMode = FALSE + BatchBuggy = FALSE +INVARIANTS + SyncPointPerSrc diff --git a/tla/MCBuggyLast.cfg b/tla/MCBuggyLast.cfg new file mode 100644 index 0000000..9457858 --- /dev/null +++ b/tla/MCBuggyLast.cfg @@ -0,0 +1,23 @@ +\* Demonstrates the cho.last allocator race: an own-source record can be +\* drained without a synchronized update to the cached local allocator state. +\* The next CommitPacket then stamps a fresh local mutation with the same +\* seq. Expected: FreshLocalIds VIOLATED. +SPECIFICATION Spec +CONSTANTS + Replicas <- MCReplicas2 + Edges <- MCEdges2 + Objects <- MCObjects + CommitBudget <- MCBudgetA2Last + GenBudget = 0 + PingBudget = 0 + BuggyRelay = FALSE + DisplaceBySrc = TRUE + DropSyncsOnClose = TRUE + ProtectLast = FALSE + EnableOwnSourceDrain = TRUE + EnablePing = FALSE + BatchMode = FALSE + BatchBuggy = FALSE +INVARIANTS + TypeOK + FreshLocalIds diff --git a/tla/MCBuggyRelay.cfg b/tla/MCBuggyRelay.cfg new file mode 100644 index 0000000..0140779 --- /dev/null +++ b/tla/MCBuggyRelay.cfg @@ -0,0 +1,26 @@ +\* Demonstrates the broadcast-relay bug of replication/sync.go Drain() +\* (fixed in this commit): a coalesced batch ending with 'B' (bye) was not +\* rebroadcast at all, so records applied at the middle replica never +\* reached downstream replicas; the subsequent re-handshake then merges the +\* origin's version vector downstream WITHOUT the data (relayed 'H' + empty +\* diff), permanently poisoning the downstream VV. +\* Expected: NoGaps (and QuiescentConverged) VIOLATED. +SPECIFICATION Spec +CONSTANTS + Replicas <- MCReplicas + Edges <- MCEdgesLine + Objects <- MCObjects + CommitBudget <- MCBudgetA3 + GenBudget = 2 + PingBudget = 0 + BuggyRelay = TRUE + DisplaceBySrc = TRUE + DropSyncsOnClose = TRUE + ProtectLast = TRUE + EnableOwnSourceDrain = FALSE + EnablePing = FALSE + BatchMode = FALSE + BatchBuggy = FALSE +INVARIANTS + NoGaps + QuiescentConverged diff --git a/tla/MCBuggyStaleSync.cfg b/tla/MCBuggyStaleSync.cfg new file mode 100644 index 0000000..af757c1 --- /dev/null +++ b/tla/MCBuggyStaleSync.cfg @@ -0,0 +1,28 @@ +\* Demonstrates the session-scoping bug found BY this model (fixed in this +\* commit by Syncer.SessionId / Chotki.AbortSyncsVia): a sync point +\* (pending diff batch) used to outlive the session whose 'H' created it. +\* A relayed 'H' creates a sync point downstream; the relayed 'D' records +\* die in the queue/channel of a reconnecting downstream link; the middle +\* replica then relays the 'V' into the NEW connection, and the downstream +\* replica applies the staged handshake VV without the data records — +\* its version vector claims ops it never stored, so nobody ever re-sends +\* them: permanent, silent data loss. +\* Expected: NoGaps VIOLATED. +SPECIFICATION Spec +CONSTANTS + Replicas <- MCReplicas + Edges <- MCEdgesLine + Objects <- MCObjects + CommitBudget <- MCBudgetA3 + GenBudget = 2 + PingBudget = 0 + BuggyRelay = FALSE + DisplaceBySrc = TRUE + DropSyncsOnClose = FALSE + ProtectLast = TRUE + EnableOwnSourceDrain = FALSE + EnablePing = FALSE + BatchMode = FALSE + BatchBuggy = FALSE +INVARIANTS + NoGaps diff --git a/tla/MCChotkiSync.tla b/tla/MCChotkiSync.tla new file mode 100644 index 0000000..13dbac8 --- /dev/null +++ b/tla/MCChotkiSync.tla @@ -0,0 +1,24 @@ +--------------------------- MODULE MCChotkiSync --------------------------- +(* Concrete constants for TLC model checking of ChotkiSync. *) +(* The .cfg files in this directory pick between these definitions. *) +EXTENDS ChotkiSync + +\* A three-replica chain a -- b -- c: the smallest topology that +\* exercises broadcast relaying (H/D/V forwarding through b). +MCReplicas == {"a", "b", "c"} +MCEdgesLine == {{"a", "b"}, {"b", "c"}} +\* The two end replicas commit one op each; the middle one only relays. +MCBudgetAC == [r \in MCReplicas |-> IF r = "b" THEN 0 ELSE 1] +\* Only "a" commits (smaller model, used for the bug demos). +MCBudgetA3 == [r \in MCReplicas |-> IF r = "a" THEN 1 ELSE 0] + +\* A two-replica model for the ping/pong machinery. +MCReplicas2 == {"a", "b"} +MCEdges2 == {{"a", "b"}} +MCBudgetA2 == [r \in MCReplicas2 |-> IF r = "a" THEN 1 ELSE 0] +\* A tiny local allocator model: one own-source drain plus one commit. +MCBudgetA2Last == [r \in MCReplicas2 |-> IF r = "a" THEN 2 ELSE 0] + +MCObjects == {"o1"} + +============================================================================= diff --git a/tla/MCFixed.cfg b/tla/MCFixed.cfg new file mode 100644 index 0000000..6d95984 --- /dev/null +++ b/tla/MCFixed.cfg @@ -0,0 +1,31 @@ +\* Main safety configuration: fixed protocol, three replicas in a chain, +\* the two end replicas commit one op each (relay in both directions +\* through "b"). Session churn is covered separately by MCFixedChurn.cfg. +\* Expected: no violations. +SPECIFICATION Spec +CONSTANTS + Replicas <- MCReplicas + Edges <- MCEdgesLine + Objects <- MCObjects + CommitBudget <- MCBudgetAC + GenBudget = 1 + PingBudget = 0 + BuggyRelay = FALSE + DisplaceBySrc = TRUE + DropSyncsOnClose = TRUE + ProtectLast = TRUE + EnableOwnSourceDrain = FALSE + EnablePing = FALSE + BatchMode = FALSE + BatchBuggy = FALSE +INVARIANTS + TypeOK + NoGaps + VVBounded + LastCoversOwnVV + LastNotAhead + FreshLocalIds + BvvExact + BvvCovers + SyncPointPerSrc + QuiescentConverged diff --git a/tla/MCFixedChurn.cfg b/tla/MCFixedChurn.cfg new file mode 100644 index 0000000..86bd5f9 --- /dev/null +++ b/tla/MCFixedChurn.cfg @@ -0,0 +1,32 @@ +\* Session churn: one committer, graceful closes, coalesced byes, +\* reconnects, re-handshakes over stale sync points. This is the same +\* state space in which MCBuggyRelay.cfg / MCBuggyDisplace.cfg find their +\* violations, explored with the fixed protocol. +\* Expected: no violations. +SPECIFICATION Spec +CONSTANTS + Replicas <- MCReplicas + Edges <- MCEdgesLine + Objects <- MCObjects + CommitBudget <- MCBudgetA3 + GenBudget = 2 + PingBudget = 0 + BuggyRelay = FALSE + DisplaceBySrc = TRUE + DropSyncsOnClose = TRUE + ProtectLast = TRUE + EnableOwnSourceDrain = FALSE + EnablePing = FALSE + BatchMode = FALSE + BatchBuggy = FALSE +INVARIANTS + TypeOK + NoGaps + VVBounded + LastCoversOwnVV + LastNotAhead + FreshLocalIds + BvvExact + BvvCovers + SyncPointPerSrc + QuiescentConverged diff --git a/tla/MCFixedLast.cfg b/tla/MCFixedLast.cfg new file mode 100644 index 0000000..edb7951 --- /dev/null +++ b/tla/MCFixedLast.cfg @@ -0,0 +1,28 @@ +\* Intended fixed cho.last allocator: the same local own-source drain witness +\* is enabled, but cho.last is synchronized before the next CommitPacket +\* reads it. Expected: no violations. +SPECIFICATION Spec +CONSTANTS + Replicas <- MCReplicas2 + Edges <- MCEdges2 + Objects <- MCObjects + CommitBudget <- MCBudgetA2Last + GenBudget = 0 + PingBudget = 0 + BuggyRelay = FALSE + DisplaceBySrc = TRUE + DropSyncsOnClose = TRUE + ProtectLast = TRUE + EnableOwnSourceDrain = TRUE + EnablePing = FALSE + BatchMode = FALSE + BatchBuggy = FALSE +INVARIANTS + TypeOK + NoGaps + VVBounded + LastCoversOwnVV + LastNotAhead + FreshLocalIds + BvvExact + BvvCovers diff --git a/tla/MCPing.cfg b/tla/MCPing.cfg new file mode 100644 index 0000000..ff33052 --- /dev/null +++ b/tla/MCPing.cfg @@ -0,0 +1,29 @@ +\* Ping/pong keep-alive machinery on a two-replica model. +\* Expected: no violations. +SPECIFICATION Spec +CONSTANTS + Replicas <- MCReplicas2 + Edges <- MCEdges2 + Objects <- MCObjects + CommitBudget <- MCBudgetA2 + GenBudget = 2 + PingBudget = 1 + BuggyRelay = FALSE + DisplaceBySrc = TRUE + DropSyncsOnClose = TRUE + ProtectLast = TRUE + EnableOwnSourceDrain = FALSE + EnablePing = TRUE + BatchMode = FALSE + BatchBuggy = FALSE +INVARIANTS + TypeOK + NoGaps + VVBounded + LastCoversOwnVV + LastNotAhead + FreshLocalIds + BvvExact + BvvCovers + SyncPointPerSrc + QuiescentConverged diff --git a/tla/MCWitness.cfg b/tla/MCWitness.cfg new file mode 100644 index 0000000..24ee3ef --- /dev/null +++ b/tla/MCWitness.cfg @@ -0,0 +1,24 @@ +\* Reachability witness: TLC is asked to "violate" NotConverged, i.e. to +\* find a behaviour in which every replica committed its budget, the +\* network went quiescent and all stores are equal. The reported +\* "counterexample" is a full happy-path trace of the protocol +\* (handshake -> diff -> V -> live relay) ending in convergence. +\* Expected: NotConverged VIOLATED (this is the desired outcome). +SPECIFICATION Spec +CONSTANTS + Replicas <- MCReplicas + Edges <- MCEdgesLine + Objects <- MCObjects + CommitBudget <- MCBudgetAC + GenBudget = 1 + PingBudget = 0 + BuggyRelay = FALSE + DisplaceBySrc = TRUE + DropSyncsOnClose = TRUE + ProtectLast = TRUE + EnableOwnSourceDrain = FALSE + EnablePing = FALSE + BatchMode = FALSE + BatchBuggy = FALSE +INVARIANTS + NotConverged diff --git a/tla/README.md b/tla/README.md new file mode 100644 index 0000000..14ab8ff --- /dev/null +++ b/tla/README.md @@ -0,0 +1,230 @@ +# TLA+ specification of the Chotki sync protocol + +`ChotkiSync.tla` is a formal model of the replication protocol implemented in +`replication/sync.go`, `chotki.go` (`drain`/`Broadcast`/`CommitPacket`), +`packets.go` (`ApplyH`/`ApplyD`/`ApplyV`/`ApplyE`) and `network/peer.go` +(read-side record coalescing). See the header comment of the module for the +precise abstraction choices. + +## What is modelled + +- **Replica state**: the pebble DB is reduced to a set of applied ops, + the global version vector (`VKey0`) and per-block version vectors + (`VKey(block)`; one object == one sync block). +- **Local id allocation**: `cho.last` is modelled as the cached allocator + state used by `CommitPacket`, with a small witness action for own-source + records drained outside the local commit lock. +- **Session FSM**: the `Syncer` feed states (`SendHandshake → SendDiff → + SendLive → SendEOF → SendNone`, plus ping/pong) and drain states, exactly + as driven by `Feed()`/`Drain()` including the `LastLit` batch-tail logic. +- **Diff sync**: pebble snapshot at handshake, per-block change detection + (`getVVChanges`: block VV vs peer global VV), diff parcels, the closing + `'V'` packet, and the sync-point batch on the receiving side that is + applied atomically when `'V'` arrives (`cho.syncs`). +- **Live sync and relaying**: `CommitPacket` broadcast, and the rebroadcast + of drained `'H'`/`'D'`/`'V'`/live records to all other sessions, which is + how a diff sync propagates through a tree of replicas. +- **The network**: per-direction ordered reliable channels; a `Drain` call + consumes an arbitrary non-empty *prefix* of the channel, modelling + `keepRead` coalescing several writes into one batch (this is essential — + several protocol decisions look only at the last record of a batch). +- **Session termination and reconnects**: `'B'` (bye) records, connection + teardown dropping whatever was still in flight or queued, and reconnects + with a fresh handshake while stale sync points survive at the replicas. + +Not modelled: sync-mode negotiation (chotki always runs `SyncRWLive` +sessions), `MaxParcelSize` truncation, `cleanSyncs` expiry (subsumed by the +session-death nondeterminism), byte-level TLV encoding, and queue overflow +(an overflow kills the session, which the model covers as session death). + +## Checked properties + +| Property | Meaning | +|---|---| +| `TypeOK` | domains of all state variables | +| `NoGaps` | **data safety**: a replica's global VV never claims an op that is not in its store. Because diff sync uses the peer's global VV as the resend floor, a violation is *permanent, silent data loss* — nobody will ever resend that op. | +| `VVBounded` | a VV never runs ahead of what the source actually committed | +| `LastCoversOwnVV` | the cached local allocator `cho.last` never lags behind the replica's own VV entry | +| `LastNotAhead` | ...nor runs *ahead* of the persisted own VV entry. `cho.last` lives only in memory and is rebuilt from the VV on restart, so an id it holds that the VV does not is reissued after a crash. With `LastCoversOwnVV` this pins `cho.last = gvv[r][r]`. | +| `FreshLocalIds` | each local own-source event receives a fresh seq; repeated seqs violate this | +| `BvvExact`, `BvvCovers` | block VVs are exact and complete w.r.t. the store; the sender-side `hasChanges` check depends on this | +| `SyncPointPerSrc` | at most one pending diff batch per origin replica (`"allow only 1 diff sync per src"`) | +| `QuiescentConverged` | when nothing is in flight or queued and all sessions are live, all replicas hold the same data (eventual consistency over a tree topology) | +| `NotConverged` | used *negated* as a reachability witness: TLC's "counterexample" is a complete happy-path trace ending in full convergence | + +## Running + +Get `tla2tools.jar` (TLA+ tools, TLC ≥ 2.15) and run from this directory: + +```sh +java -cp tla2tools.jar tlc2.TLC -config MCFixed.cfg -workers auto -deadlock MCChotkiSync +``` + +| Config | Expectation | +|---|---| +| `MCFixed.cfg` | fixed protocol, chain `a–b–c`, both ends commit: **no violations** | +| `MCFixedChurn.cfg` | fixed protocol + session closes/reconnects: **no violations** | +| `MCFixedLast.cfg` | intended fixed `cho.last` allocator witness: **no violations** | +| `MCWitness.cfg` | `NotConverged` "violated": a full convergence trace | +| `MCPing.cfg` | ping/pong machinery enabled: no violations | +| `MCBuggyRelay.cfg` | pre-fix relay logic: **`NoGaps` violated** (bug 1 below) | +| `MCBuggyDisplace.cfg` | pre-fix displacement: **`SyncPointPerSrc` violated** (bug 2) | +| `MCBuggyStaleSync.cfg` | pre-fix sync-point lifetime: **`NoGaps` violated** (bug 3) | +| `MCBuggyLast.cfg` | pre-fix unsynchronized `cho.last`: **`FreshLocalIds` violated** (bug 4) | +| `MCBatchFixed.cfg` | fixed batched drain (`BatchMode`), chain `a–b–c`: **no violations** (batching preserves `NoGaps`/`QuiescentConverged`) | +| `MCBatchFixedLast.cfg` | fixed batched drain + own-source drain witness: **no violations** (`cho.last` stays in lock-step with the VV) | +| `MCBatchBuggy.cfg` | pre-fix batched drain (`BatchBuggy`): **`LastNotAhead` violated** (bug 5 below) | + +(`-deadlock` disables deadlock reporting: behaviours legitimately terminate +once the commit/reconnect budgets are exhausted.) + +## Bugs found while writing this spec + +The original model-switchable sync bugs plus the added local allocator +witness: + +1. **Lost rebroadcast of applied records** (`replication/sync.go Drain`, + modelled by `BuggyRelay = TRUE`). When network batching coalesced records + with the peer's closing `'B'` (bye) — e.g. `[E, B]` in live mode or + `[D, V, B]` at the end of a non-live diff — the batch was applied locally + but **not rebroadcast at all**; the same happened to the applied prefix of + a batch that failed mid-way. Downstream replicas never receive those + records: live records are not re-sent, and later diff syncs skip them + because the middle replica already has them. Worse, at the next + re-handshake the relayed `'H'` merges the origin's version vector + downstream while the following (empty) diff carries no data, so the + downstream VV gets poisoned and the loss becomes permanent and silent — + TLC finds a 27-state trace ending with `gvv[c][a] = 1` and `store[c] = {}`. + Fix: rebroadcast exactly the locally-applied prefix of every drained + batch, minus the session-scoped trailing `'B'` (`relayApplied`, backed by + `DrainApplied` which reports how many records of a batch were applied). + +2. **Sync-point displacement never fired** (`chotki.go drain 'H'`, modelled + by `DisplaceBySrc = FALSE`). The `"allow only 1 diff sync per src"` + cleanup compared sync-point keys against `cho.src` — the *local* replica + id — but sync-point keys are the *origins'* snapshot ids, and a replica + never drains its own handshake, so the cleanup deleted nothing. Stale + sync points (with their pebble batches) from an origin's previous, + interrupted session survived until the `cleanSyncs` timeout. Fix: compare + against the source of the incoming handshake id. + +3. **Sync points outlived the session that fed them** (modelled by + `DropSyncsOnClose = FALSE`). This one was found *by TLC*, in a + first-draft `MCFixedChurn` run with bugs 1–2 already fixed: `cho.syncs` + is keyed only by the origin's snapshot id, so a pending diff batch + survives connection churn. TLC's 30-state counterexample: while `a—b` + diff-syncs, `b` relays `H(a)` to `c` (sync point at `c`); the `b—c` + link then gracefully reconnects, and the relayed `D(a)` dies in the + old connection's queue; `b` (whose `a`-session is still live) relays + the closing `V(a)` into the *new* `b—c` connection; `c` still holds + the stale sync point and applies it — the staged handshake VV lands in + `VKey0` with no data (`gvv[c][a] = 1`, `store[c] = {}`). In production + terms: any network blip on a downstream link while an upstream diff + sync is being relayed through it can permanently and silently lose + data downstream. Fix: every sync point remembers the replication + session that created it (`Syncer.SessionId()`, carried in the drain + context) and `Syncer.Close` aborts its sessions' sync points + (`Chotki.AbortSyncsVia`) — a late `V` on a newer connection then gets + `ErrSyncUnknown`, and the session restarts with a clean re-handshake. + +4. **`cho.last` could lag behind own-source records** (modelled by + `ProtectLast = FALSE` and `EnableOwnSourceDrain = TRUE`). `CommitPacket` + stamps a new local mutation from the cached `cho.last` value, while + `drain` can also apply records whose id source is the local replica and + advance the replica's own VV. Without a synchronization edge around that + cache, the next commit can observe stale `cho.last` and reuse an already + issued seq. The `MCBuggyLast.cfg` trace is deliberately tiny: an + own-source drain applies seq 1 but leaves `last = 0`, then `CommitPacket` + also emits seq 1, violating `FreshLocalIds`. In the Go code the two + paths that advance `cho.last` really do not share a lock: local commits + hold `commitMutex`, while replication sessions drain own-source records + (our own history synced back after a restore from an older snapshot) + under `cho.lock.RLock` only — and `rdx.ID` is two uint64s, so a torn + read is possible on top of the lost update. Fixed by giving the + allocator cache its own mutex (`lastLock`) around the commit allocation + (`nextLast`), the own-source advance in `drain`, `Last()` and the + `Close()` reset; `TestCommitAllocatorSyncedWithOwnSourceDrain` races the + two paths under `-race` and fails against the pre-fix code. + +5. **Batched drain could advance `cho.last` past the persisted VV** (modelled + by `BatchMode = TRUE` with `BatchBuggy = TRUE`). `chotki.go drain()` was + changed to accumulate the `Y/C/O/E` records of a batch into one pebble + `pb` applied once after the loop (one write per drain), instead of + per-packet. The first draft advanced `cho.last` to an own-source record's + id *inside the loop* — before that durable write — and dropped `pb` + entirely on a mid-batch error (a bad packet, `ErrSyncUnknown` after a sync + point timed out, an unsupported type). The record's version-vector bump + was dropped with `pb`, but `cho.last` stayed advanced: `cho.last` now + *leads* the persisted VV. Since `cho.last` is rebuilt from the VV on + restart (`cho.last = vv.GetID(cho.src)` in `Open`), a crash before a + resync regresses it, and the next `CommitPacket` reissues an id a peer that + received the drained/relayed record already holds — divergence. The + `MCBatchBuggy.cfg` trace is one step: a dropped own-source drain advances + `last` to 1 while `gvv` stays 0, violating `LastNotAhead`. Fixed by keeping + the **drop-on-error** semantics — a mid-batch error drops the whole `pb`, so + no partial prefix is ever persisted, `applied` is 0 and nothing non-durable + is relayed — while advancing `cho.last` (under `lastLock`) only *lazily*, + after the durable write, so a dropped batch consumes no id and `cho.last` + can never lead the VV. (The earlier draft instead *flushed* the applied + prefix on error; that was reverted because it made `CommitBatch` no longer + all-or-nothing, doubling a Z-counter's read-modify-write delta on chunk retry.) + `TestDrainErrorDoesNotOverrunAllocator` drives a mixed + `[own-source C, unknown-sync V]` batch and asserts the dropped batch persists + nothing; it fails against both the eager-`cho.last` bug and the old flush + behaviour. `MCBatchFixed`/`MCBatchFixedLast` re-check the whole property set + with `BatchMode = TRUE` (drop-on-error + lazy `cho.last`); `MCBatchBuggy` + keeps the eager-`cho.last` variant as the negative witness. + +Bugs found in the same code while studying it for the model (also fixed, not +modelled at the byte/timer level): + +3. `processPings` removed `'P'` records while ranging over the same slice, + skipping the record that followed a removed one (a coalesced `[ping, X]` + batch left `X` unprocessed by the ping logic and could relay a stray + record); rewritten as an in-place filter. +4. `WaitDrainState` leaked one goroutine per call whenever the context could + not be cancelled (`Feed`'s `SendNone` path passes `context.Background()`, + so every closed session leaked a goroutine blocked on `ctx.Done()`), and + another goroutine whenever the caller abandoned the wait on timeout (the + result channel was unbuffered); fixed with an internal cancel and a + buffered channel. +5. `Feed`'s `SendPing` case mutated `sync.pingTimer` without holding the + lock, racing `resetPingTimer` on the drain side; now locked. +6. `ApplyD`/`ApplyV` could spin forever on a truncated inner TLV record + (`protocol.Take` returns the input unchanged on incomplete data) and + `ApplyV` could overwrite an earlier `ErrBadVPacket` with a later + successful merge; both now fail fast. A malformed relayed handshake could + also make `ApplyH` merge a nil VV into `VKey0`. +7. `drain`'s `'H'` case stored the sync point (and previously leaked the + displaced batch) even when `ApplyH` failed; the batch is now closed and + the sync point only registered on success. +8. A batch that became empty after ping filtering went through the + `LastLit`-based state transition with `LastLit == 0` and could flip the + drain FSM to `SendLive` prematurely (or panic on `recs[0:1]` in the + handshake state); such batches now return early. +9. `Syncer.Close` closed the pebble snapshot and its iterators while a + concurrent `Feed` (`FeedBlockDiff`) could still be using them — a data + race that `TestSyncPointRace_ReconnectionStorm -race` turns into a + `mergingIter` panic; snapshot/iterator lifetime is now guarded by a + dedicated mutex (`snapLock`). +10. `Feed`'s `SendNone` case waited for the drain state to reach + `SendNone`, unblocking itself via a one-shot timer that forced the + state — but a `Drain` racing with `Close` (e.g. a late handshake) + could move the drain state backwards *after* the timer had fired, + leaving the feed goroutine blocked forever (the reconnection-storm + test hangs on this both before and after the other fixes). The + lingering wait is now bounded by a context timeout instead. +11. Draining a peer's `'B'` (bye) set the drain state to `SendNone`, and + `Feed`'s next call surrendered to that state *immediately* — even in + the middle of its own diff. In a bidirectional non-live sync the + faster side's bye could therefore cut the slower side's diff short of + its `'V'` packet, stranding a staged diff batch at the peer that + silently misses the promised data (`TestChotki_Sync3` flakes on + exactly this). A bye only means the peer has nothing more to *send*; + its drain side keeps working until the connection closes, so `Feed` + now finishes its handshake/diff phase before winding down. +12. `examples/plain_object_test.go` never closed its third DB handle, so + pebble's background compactions raced the deferred `os.RemoveAll` and + failed the test after it had passed; `chotki_index_test.go` waited + only 5s for the background reindex worker, which a loaded `-race` run + can exceed (now 30s; the helper polls, so fast machines stay fast).