Merge pull request 'Switch the memory store's failure under its lock, so a test cannot race the keeper' (#148) from fix/conditions-store-race into main

cmd/mesh-controller/conditions_test.go

@@ -86,7 +86,7 @@ func TestASilenceThroughTheVerbHoldsAndTheConditionStaysOpen(t *testing.T) {
 func TestUnreadableConditionsAreNotAWellMesh(t *testing.T) {
 	open := aMesh(t)
 	_, store := withConditionsInMemory(t)
-	store.Fail = errors.New("the bus is away")
+	store.SetFail(errors.New("the bus is away"))
 	asked, err := theThreeQuestions(t.Context(), open)
 	if err != nil {
 		t.Fatal(err)
@@ -98,7 +98,7 @@ func TestUnreadableConditionsAreNotAWellMesh(t *testing.T) {
 	if !strings.HasPrefix(said, "the open conditions could NOT be read") || strings.Contains(said, "no open conditions") {
 		t.Fatalf("%s", said)
 	}
-	store.Fail = nil
+	store.SetFail(nil)
 	asked, _ = theThreeQuestions(t.Context(), open)
 	said = printed(t, func() error { return printStatus(asked) })
 	if asked.well() && !strings.Contains(said, "no open conditions;") {

internal/conditions/memory.go

@@ -16,18 +16,27 @@ type InMemory struct {
 	values   map[string]Entry
 	revision uint64
 	events   []Event
-	// Fail, when set, is what every read and write answers: a store that is away.
+	// fail, when set, is what every read and write answers: a store that is away. It is set only
-	Fail error
+	// through SetFail, under the lock, because the keeper's telling goroutine reads it while a test
+	// takes the store away.
+	fail error
 }
 
 // NewInMemory is an empty store.
 func NewInMemory() *InMemory { return &InMemory{values: map[string]Entry{}} }
 
+// SetFail makes every read and write answer err from now on, or none when err is nil.
+func (m *InMemory) SetFail(err error) {
+	m.mu.Lock()
+	defer m.mu.Unlock()
+	m.fail = err
+}
+
 func (m *InMemory) Get(_ context.Context, key string) (Entry, bool, error) {
 	m.mu.Lock()
 	defer m.mu.Unlock()
-	if m.Fail != nil {
+	if m.fail != nil {
-		return Entry{}, false, m.Fail
+		return Entry{}, false, m.fail
 	}
 	e, ok := m.values[key]
 	return e, ok, nil
@@ -36,8 +45,8 @@ func (m *InMemory) Get(_ context.Context, key string) (Entry, bool, error) {
 func (m *InMemory) Create(_ context.Context, key string, value []byte) error {
 	m.mu.Lock()
 	defer m.mu.Unlock()
-	if m.Fail != nil {
+	if m.fail != nil {
-		return m.Fail
+		return m.fail
 	}
 	if _, ok := m.values[key]; ok {
 		return ErrMoved
@@ -50,8 +59,8 @@ func (m *InMemory) Create(_ context.Context, key string, value []byte) error {
 func (m *InMemory) Update(_ context.Context, key string, value []byte, revision uint64) error {
 	m.mu.Lock()
 	defer m.mu.Unlock()
-	if m.Fail != nil {
+	if m.fail != nil {
-		return m.Fail
+		return m.fail
 	}
 	if e, ok := m.values[key]; !ok || e.Revision != revision {
 		return ErrMoved
@@ -64,8 +73,8 @@ func (m *InMemory) Update(_ context.Context, key string, value []byte, revision
 func (m *InMemory) Delete(_ context.Context, key string, revision uint64) error {
 	m.mu.Lock()
 	defer m.mu.Unlock()
-	if m.Fail != nil {
+	if m.fail != nil {
-		return m.Fail
+		return m.fail
 	}
 	if e, ok := m.values[key]; !ok || e.Revision != revision {
 		return ErrMoved
@@ -77,8 +86,8 @@ func (m *InMemory) Delete(_ context.Context, key string, revision uint64) error
 func (m *InMemory) All(context.Context) (map[string]Entry, error) {
 	m.mu.Lock()
 	defer m.mu.Unlock()
-	if m.Fail != nil {
+	if m.fail != nil {
-		return nil, m.Fail
+		return nil, m.fail
 	}
 	out := make(map[string]Entry, len(m.values))
 	for k, v := range m.values {
@@ -90,8 +99,8 @@ func (m *InMemory) All(context.Context) (map[string]Entry, error) {
 func (m *InMemory) Append(_ context.Context, e Event) error {
 	m.mu.Lock()
 	defer m.mu.Unlock()
-	if m.Fail != nil {
+	if m.fail != nil {
-		return m.Fail
+		return m.fail
 	}
 	m.events = append(m.events, e)
 	return nil
@@ -100,8 +109,8 @@ func (m *InMemory) Append(_ context.Context, e Event) error {
 func (m *InMemory) Since(_ context.Context, since time.Time) ([]Event, error) {
 	m.mu.Lock()
 	defer m.mu.Unlock()
-	if m.Fail != nil {
+	if m.fail != nil {
-		return nil, m.Fail
+		return nil, m.fail
 	}
 	var out []Event
 	for _, e := range m.events {
@@ -118,14 +127,22 @@ type Told struct {
 	mu     sync.Mutex
 	Events []Event
 	Names  []string
-	Fail   error
+	// fail, when set, is what every publish answers; set only through SetFail, under the lock.
+	fail error
+}
+
+// SetFail makes every publish answer err from now on, or none when err is nil.
+func (t *Told) SetFail(err error) {
+	t.mu.Lock()
+	defer t.mu.Unlock()
+	t.fail = err
 }
 
 func (t *Told) PublishSeatEvent(_ context.Context, seat, event string, body []byte) error {
 	t.mu.Lock()
 	defer t.mu.Unlock()
-	if t.Fail != nil {
+	if t.fail != nil {
-		return t.Fail
+		return t.fail
 	}
 	if seat != Seat {
 		return errors.New("told under the wrong seat: " + seat)

internal/conditions/memory_test.go

@@ -0,0 +1,30 @@
+package conditions
+
+import (
+	"errors"
+	"fmt"
+	"testing"
+)
+
+// **A test may take the store away while the keeper is still telling.** The keeper keeps every
+// transition from a goroutine of its own, so the memory store's failure is read there while a test
+// switches it: it is switched under the store's lock, or the race detector fails the suite at random
+// (it once failed TestAnUnreadableStoreClearsNothing). A hundred transitions still being kept while
+// the failure is switched a hundred times makes the race certain, not rare, when the lock is skipped.
+func TestTheStoreIsTakenAwayWhileTheKeeperIsTelling(t *testing.T) {
+	k, store, told, _ := keeper(t)
+	ctx := t.Context()
+	const n = 100
+	for i := range n {
+		if _, err := k.Observe(ctx, silent(fmt.Sprintf("m%d", i))); err != nil {
+			t.Fatal(err)
+		}
+	}
+	for range n {
+		store.SetFail(errors.New("the bus is away"))
+		store.SetFail(nil)
+	}
+	told.SetFail(errors.New("no responders"))
+	told.SetFail(nil)
+	settled(t, told, n)
+}

internal/conditions/store_test.go

@@ -199,15 +199,17 @@ func TestAnUnreadableStoreClearsNothing(t *testing.T) {
 	if _, err := k.Observe(ctx, silent("ace")); err != nil {
 		t.Fatal(err)
 	}
-	store.Fail = errors.New("the bus is away")
+	store.SetFail(errors.New("the bus is away"))
 	if err := k.Reconcile(ctx, "S1", nil); err == nil {
 		t.Fatal("reconciled against a store it could not read")
 	}
 	if _, err := k.Open(ctx); err == nil {
 		t.Fatal("an unreadable store answered as read")
 	}
-	store.Fail = nil
+	store.SetFail(nil)
+	store.mu.Lock()
 	store.values["machine.g14.silent"] = Entry{Value: []byte("{not a condition"), Revision: 99}
+	store.mu.Unlock()
 	if _, err := k.Open(ctx); err == nil || !strings.Contains(err.Error(), "machine.g14.silent") {
 		t.Fatalf("an unreadable condition was left out rather than said: %v", err)
 	}
@@ -362,16 +364,15 @@ func TestTheEventShapeIsTheContract(t *testing.T) {
 
 // **A transition the bus will not take is offered again**, and said lost only after TellFor.
 func TestATransitionIsOfferedAgainWhileTheBusIsAway(t *testing.T) {
-	store, told, c := NewInMemory(), &Told{Fail: errors.New("no responders")}, newClock()
+	store, told, c := NewInMemory(), &Told{}, newClock()
+	told.SetFail(errors.New("no responders"))
 	k := NewKeeper(t.Context(), Options{Store: store, History: store, Teller: told, Now: c.now})
 	defer k.Close(context.Background())
 	if _, err := k.Observe(t.Context(), silent("ace")); err != nil {
 		t.Fatal(err)
 	}
 	time.Sleep(300 * time.Millisecond)
-	told.mu.Lock()
+	told.SetFail(nil)
-	told.Fail = nil
-	told.mu.Unlock()
 	said := settled(t, told, 1)
 	if said[0].Key != "machine.ace.silent" || k.Unsaid() != 0 {
 		t.Fatalf("said %+v, unsaid %d", said, k.Unsaid())