take Cache.mx before bbolt's writer in WithBatch — publish/fill deadlock

Author: mrpalide

cmd/apps/skychat/pairing/pair.go

@@ -256,31 +256,30 @@ func Open(cfg Config) (*Pair, error) {
 //
 // # Why this hangs off Send instead of a timer
 //
-// The announcement is a CXO write, and pkg/cxo/skyobject has a
+// The announcement is a CXO write, and pkg/cxo/skyobject HAD a
 // lock-order inversion between a write and a fill on the SAME node:
-// Cache.Get holds Cache.mx and blocks on bbolt's writer lock, while
-// Cache.WithBatch (reached only from the publisher's tree walk) holds
-// bbolt's writer lock and blocks on Cache.mx. A pair puts its publisher
+// Cache.Get held Cache.mx and blocked on bbolt's writer lock, while
+// Cache.WithBatch (reached only from the publisher's tree walk) held
+// bbolt's writer lock and blocked on Cache.mx. A pair puts its publisher
 // and its subscriber on one node by design, so an announcement
-// published while the peer's tree is filling can wedge the node
+// published while the peer's tree was filling could wedge the node
 // permanently — the publish loop stops clearing its dirty flag and
 // every later send is silently lost.
 //
 // Confirmed rather than guessed: with Cache.WithBatch bypassed the
 // pairing suite ran 6/6 green, and with it a background announce timer
 // hung the suite roughly one run in four.
 //
-// Riding Send is what makes this safe by construction rather than by
-// timing. The Put lands in the same publisher batch as the message, so
+// That inversion is fixed — WithBatch now takes Cache.mx before the
+// writer, pinned by TestWithBatch_DoesNotInvertCacheAndWriterLocks — so
+// a timer is no longer unsafe. Riding Send is kept anyway, on its own
+// merits: the Put lands in the same publisher batch as the message, so
 // the pair performs exactly as many publishes as it did before this
-// feature existed — no new window is opened. The cost is that forward
-// secrecy engages once each side has sent at least once (the first
-// message from each side goes out under the legacy static key), which
-// for a conversation is the normal case.
-//
-// Removing this workaround is a one-line change once the CXO lock order
-// is fixed; the announcement can then go on a timer and cover pairs
-// where one side never speaks.
+// feature existed, and no timing assumption is involved at all. The
+// cost is that forward secrecy engages once each side has sent at least
+// once (the first message from each side goes out under the legacy
+// static key), which for a conversation is the normal case. A timer
+// would only add value for pairs where one side never speaks.
 func (p *Pair) maybeAnnounce(now time.Time) {
 	if p.pub == nil {
 		return

pkg/cxo/skyobject/cache.go

@@ -129,9 +129,13 @@ type Cache struct {
 	// Reads happen under c.mx (every Cache write/read path holds it),
 	// so concurrent Set/Get/Inc callers serialize their tx use through
 	// the mutex; the bbolt tx is never touched by two goroutines at
-	// once. WithBatch tears the pin down only after taking c.mx again
-	// to drain any in-flight CXDS user — see WithBatch for the full
-	// invariant.
+	// once.
+	//
+	// The pin is established and torn down so that EVERY goroutine
+	// holding c.mx during the tx's lifetime sees it. That is not a
+	// nicety: a c.mx holder that misses the pin falls through to the
+	// unpinned store and blocks on bbolt's writer lock, which the tx
+	// owns — while the tx owner blocks on c.mx. See WithBatch.
 	batchTx atomic.Pointer[data.CXDS]
 }
 
@@ -196,34 +200,78 @@ func (c *Cache) db() data.CXDS {
 // that one tx — replacing N per-leaf commits with a single one. fn
 // returning nil commits the tx; a non-nil return rolls it back.
 //
-// The pin lives in c.batchTx (atomic). Reads happen under c.mx, so
-// concurrent Cache writers/readers from outside fn naturally serialize
-// against fn's tx use through the mutex — the bbolt tx is never
-// touched by two goroutines at once. The tear-down sequence on the
-// way out:
+// LOCK ORDER — the reason this function is shaped the way it is.
+//
+// Two locks are in play: c.mx and bbolt's writer lock (held for the
+// whole of DB.Update / DB.Batch). Everywhere else in the Cache the
+// order is c.mx → writer: Cache.Get(key, inc != 0) and Cache.Set on a
+// miss take c.mx and then reach bbolt, which needs the writer for the
+// refcount bump. This function is the one place that would take them
+// the other way round — RunBatch opens the tx (writer) and fn then
+// calls back into Cache.Set (c.mx) — and that inversion deadlocked:
+//
+//	G1 fill:    holds c.mx        → waits for the writer (DB.Batch)
+//	G2 publish: holds the writer  → waits for c.mx (Cache.Set in fn)
+//
+// It wedged the node permanently: the publisher never cleared dirty,
+// every later send was silently lost, and teardown hung because the
+// cleanup sweep also wants c.mx. Windows CI hit it as a 25-minute
+// package timeout in cmd/apps/skychat/pairing, which publishes and
+// subscribes on one node by design.
+//
+// The fix is to make c.mx → writer hold here too, WITHOUT holding c.mx
+// across fn (fn's own Cache.Set needs it, and the mutex isn't
+// reentrant). Three windows, in order:
+//
+//  1. Take c.mx BEFORE opening the tx. Anyone already inside the Cache
+//     — possibly blocked in bbolt — finishes first, so we never open
+//     the writer underneath a c.mx holder that still needs it.
+//  2. Store the pin, then release c.mx. From here every arriving Cache
+//     op sees the pin and joins our tx, so none of them touches the
+//     writer while we own it. fn runs in this window.
+//  3. Re-take c.mx BEFORE clearing the pin. Taking it is safe (by (2)
+//     no c.mx holder is waiting on the writer) and it drains anyone
+//     mid-call on the scoped handle; clearing the pin only afterwards
+//     means no goroutine can pick the unpinned store up while the tx
+//     is still open. c.mx is released after RunBatch returns, i.e.
+//     after the commit.
 //
-//  1. Store nil into c.batchTx so any newly-arriving Cache.Set sees
-//     the untwisted CXDS (and will block in bbolt's writer mutex
-//     until our tx commits, as it would have pre-batch).
-//  2. Lock/unlock c.mx once to drain any in-flight Cache.Set that
-//     captured the now-stale tx pointer before step 1 — they finish
-//     their work under our serialization while we wait.
-//  3. Return; the bbolt tx commits at this point.
+// The cost of step (1) is that Cache operations now also queue behind
+// however long the writer takes to become free — including a long
+// RemoveObjects sweep. Before, an inc==0 read (bbolt View, no writer)
+// could slip past. That is latency, not a cycle: nothing that holds the
+// writer ever asks for c.mx (RemoveObjects deliberately snapshots
+// CachedKeys up front for this reason), so the wait always ends.
 //
 // fn must not retain the pinned handle past its return — the tx is
-// gone afterward.
+// gone afterward. WithBatch is not reentrant (step 1 would deadlock
+// against itself); it has a single caller, treestore.publishRoot.
 func (c *Cache) WithBatch(fn func() error) error {
+	// (1) Claim the Cache before the writer. RunBatch invokes fn
+	// synchronously on this goroutine, so this plain bool needs no
+	// synchronization of its own.
+	c.mx.Lock()
+	held := true
+	unlock := func() {
+		if held {
+			held = false
+			c.mx.Unlock()
+		}
+	}
+	// Covers step (3)'s re-lock, and the case where RunBatch fails
+	// before ever calling us back.
+	defer unlock()
+
 	return c.c.db.CXDS().RunBatch(func(scoped data.CXDS) (err error) {
 		c.batchTx.Store(&scoped)
+		// (2) The pin is visible to everyone who takes c.mx from here
+		// on; hand the Cache back to them.
+		unlock()
 		defer func() {
-			c.batchTx.Store(nil)
-			// Wait for any in-flight Cache operation that may still be
-			// using the scoped CXDS to finish before letting bbolt
-			// commit. Holding the mutex briefly is sufficient: every
-			// Cache CXDS call holds c.mx for its duration, so once we
-			// acquire it nobody else is mid-tx.
+			// (3) Drain in-flight scoped users, then retire the pin.
 			c.mx.Lock()
-			c.mx.Unlock() //nolint:staticcheck // intentional drain
+			held = true
+			c.batchTx.Store(nil)
 		}()
 		return fn()
 	})

pkg/cxo/skyobject/cache_batch_lockorder_test.go

@@ -0,0 +1,162 @@
+// Package skyobject pkg/cxo/skyobject/cache_batch_lockorder_test.go
+//
+// Pins the lock order between Cache.mx and bbolt's writer lock, which
+// WithBatch is the only place able to invert (it opens the writer, then
+// calls back into Cache.Set). The inversion deadlocked a node
+// permanently — the publisher stopped clearing dirty, every later send
+// was silently dropped, and teardown hung because the cleanup sweep
+// wants Cache.mx too. Windows CI hit it as a 25-minute package timeout
+// in cmd/apps/skychat/pairing, which publishes and subscribes on one
+// CXO node by design.
+//
+// This test drives the interleaving directly rather than waiting for a
+// pairing suite to get unlucky: hold the writer, park a cache reader
+// that needs it (a refcount bump goes through a write tx), then start
+// the batch on top of both.
+package skyobject
+
+import (
+	"fmt"
+	"path/filepath"
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/skycoin/skycoin/src/cipher"
+
+	"github.com/skycoin/skywire/pkg/cxo/data"
+)
+
+// lockOrderRounds — one round is enough against the pre-fix code (the
+// staging in lockOrderRound is deliberate, not a race to lose), but the
+// staging leans on two short sleeps, so a few rounds keep it honest on a
+// loaded runner where one of them might land wrong.
+const lockOrderRounds = 5
+
+// lockOrderWatchdog bounds the whole run. Far above the ~1s the rounds
+// take when nothing is wedged — it only has to tell "slow CI" from
+// "blocked forever".
+const lockOrderWatchdog = 30 * time.Second
+
+// TestWithBatch_DoesNotInvertCacheAndWriterLocks fails (by watchdog) if
+// WithBatch ever again takes bbolt's writer lock before Cache.mx.
+func TestWithBatch_DoesNotInvertCacheAndWriterLocks(t *testing.T) {
+	// An on-disk container: the in-memory CXDS has no writer lock, so
+	// the inversion this guards against cannot exist there.
+	conf := NewConfig()
+	conf.InMemoryDB = false
+	conf.DBPath = filepath.Join(t.TempDir(), "db")
+
+	c, err := NewContainer(conf)
+	if err != nil {
+		t.Fatalf("NewContainer: %v", err)
+	}
+
+	done := make(chan struct{})
+	go func() {
+		defer close(done)
+		for i := 0; i < lockOrderRounds; i++ {
+			lockOrderRound(t, c, i)
+		}
+	}()
+
+	select {
+	case <-done:
+	case <-time.After(lockOrderWatchdog):
+		// Deliberately no Close() on this path: the wedged goroutines
+		// hold the bbolt writer, and bolt's Close waits for it — the
+		// cleanup would hang exactly like the bug under test.
+		t.Fatalf("deadlock: WithBatch and a concurrent Cache.Get are waiting on each other "+
+			"(Cache.mx vs the bbolt writer) — %s elapsed with rounds unfinished", lockOrderWatchdog)
+	}
+
+	if err := c.Close(); err != nil {
+		t.Errorf("Close: %v", err)
+	}
+}
+
+// lockOrderRound stages the three-goroutine interleaving that produced
+// the CI hang, then lets it resolve:
+//
+//	(a) holds bbolt's writer lock,
+//	(b) WithBatch — wants the writer, and once inside wants Cache.mx,
+//	(c) a Cache.Get with inc != 0 — takes Cache.mx, then needs the
+//	    writer for the refcount bump, so it parks holding the mutex.
+//
+// Order matters, and it is why (b) is started before (c): bbolt hands a
+// contended writer lock to the goroutine that queued first, so whoever
+// waits first wins it when (a) lets go. With (b) first, the pre-fix code
+// took the writer, pinned, and only then asked for Cache.mx — which (c)
+// was holding while waiting for the writer (b) had just taken. Cycle.
+//
+// Start them the other way round and the pre-fix code survives by luck:
+// (c)'s refcount bump commits before (b) ever enters the tx. That
+// accident is why this bug reached CI as an occasional 25-minute hang
+// instead of a reproducible failure.
+//
+// Post-fix (b) takes Cache.mx before the writer, so it simply queues
+// behind (c) — no interleaving of the three can cycle.
+func lockOrderRound(t *testing.T, c *Container, round int) {
+	t.Helper()
+
+	// A key that is on disk but NOT in the cache, so Get has to reach
+	// bbolt. Fresh per round: once round N's Get lands the object in
+	// the cache, a repeat would short-circuit before the DB.
+	key := cipher.SumSHA256([]byte(fmt.Sprintf("lock-order-probe-%d", round)))
+	if _, err := c.DB().CXDS().Set(key, []byte("probe"), 1); err != nil {
+		t.Errorf("seed CXDS: %v", err)
+		return
+	}
+
+	var wg sync.WaitGroup
+	holding, release := make(chan struct{}), make(chan struct{})
+
+	// (a) Occupy the writer lock so (b) and (c) both have to wait.
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		err := c.DB().CXDS().RunBatch(func(_ data.CXDS) error {
+			close(holding)
+			<-release
+			return nil
+		})
+		if err != nil {
+			t.Errorf("holder RunBatch: %v", err)
+		}
+	}()
+	<-holding
+
+	// (b) The publisher's batch. Queues on the writer first, so it is
+	// the one that gets it.
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		err := c.WithBatch(func() error {
+			k := cipher.SumSHA256([]byte(fmt.Sprintf("lock-order-batch-%d", round)))
+			_, err := c.Set(k, []byte("batched"), 1)
+			return err
+		})
+		if err != nil {
+			t.Errorf("WithBatch: %v", err)
+		}
+	}()
+	// No signal exists for "now blocked on the writer", so give it a
+	// moment. A too-short wait only weakens the round; it cannot cause a
+	// false failure, since a correct WithBatch never deadlocks either
+	// way.
+	time.Sleep(100 * time.Millisecond)
+
+	// (c) The cache reader. inc != 0 makes it a read-modify-write, which
+	// bbolt serves from a write tx — so it parks with Cache.mx held.
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		// The returned value and error don't matter — the locks this
+		// call takes on its way to bbolt are the whole point.
+		_, _, _ = c.Get(key, 1) //nolint:errcheck,gosec // see above
+	}()
+	time.Sleep(100 * time.Millisecond)
+
+	close(release)
+	wg.Wait()
+}