Only an online healthy witness, and only a first failure, keep a version from being held; failed-alone clears when a node leaves (ADR 0557, issue 1496)

Review of 2ee7589: a node that sleeps after running F healthy kept its stale have as a witness, so F stayed unheld while it failed on the online nodes; and a failed-alone condition was never cleared once its node left the module. A witness must now be online by the watchman's presence. F failing on a second node is no longer one node's failure: the hold and the set-back stand (ADR 0557 version 6, §2). A remembered failed-alone key whose node is no longer one of the module's nodes is cleared once; the first pass after a start clears it for every node the presence bucket knows, for a module with a failed have.
2026-10-26 22:16:38 +01:00
parent 2ee7589474
commit 16e4e61a0f
4 changed files with 111 additions and 10 deletions
@@ -3,6 +3,7 @@ package main
 import (
 	"context"
 	"errors"
+	"maps"
 	"slices"
 	"strings"
 	"time"
@@ -159,7 +160,7 @@ func (p *passer) wants(ctx context.Context) {
 		gatherNeither(neither, name, out)
 		leftNeither(left, m, wanted[name], out)
 		p.sayNoWant(name, m.Nodes, wanted[name], out)
-		p.sayNodeFailed(name, fp, m.Nodes, out, first)
+		p.sayNodeFailed(name, fp, m.Nodes, nodes, out, first)
 		if first {
 			startClears(stale, m, out)
 			if fp != "" && !m.Held && out.Held == nil && !out.Lifted {
@@ -236,14 +237,29 @@ func (p *passer) sayNoWant(module string, nodes []string, before map[string]deli

 // sayNodeFailed raises, once per node and words, the condition of a failed health check of a module's newest fingerprint fp
 // that is the node's own, since fp runs healthy elsewhere and is not held (ADR 0557 §1, issue 1496), and says it in the journal;
-// it clears the condition once when the node no longer fails alone. The first pass after a start clears it for every node that
-// does not fail alone, since a previous process may have raised it (a clear of a condition not open is quiet at the router).
-// A condition the router did not take is said again at the next pass.
-func (p *passer) sayNodeFailed(module, fp string, nodes []string, out assign.Outcome, first bool) {
+// it clears the condition once when the node no longer fails alone, or is no longer one of the module's nodes. The first pass
+// after a start remembers nothing a previous process raised: for a module with a failed have on any node it clears the
+// condition of every node the presence bucket knows that does not fail alone (a clear of a condition not open is quiet at the
+// router). A condition the router did not take is said again at the next pass.
+func (p *passer) sayNodeFailed(module, fp string, nodes []string, known assign.Nodes, out assign.Outcome, first bool) {
 	if p.nodeFailed == nil {
 		p.nodeFailed = map[string]string{}
 	}
+	look := map[string]bool{}
 	for _, node := range nodes {
+		look[node] = true
+	}
+	for key := range p.nodeFailed {
+		if node, m, _ := strings.Cut(key, "."); m == module {
+			look[node] = true
+		}
+	}
+	if first && anyFailed(known, module) {
+		for node := range known.Online {
+			look[node] = true
+		}
+	}
+	for _, node := range slices.Sorted(maps.Keys(look)) {
 		key := node + "." + module
 		f, failing := out.NodeFailed[node]
 		last, said := p.nodeFailed[key]
@@ -273,6 +289,16 @@ func (p *passer) sayNodeFailed(module, fp string, nodes []string, out assign.Out
 	}
 }

+// anyFailed says whether any node's have of module says its health failed.
+func anyFailed(n assign.Nodes, module string) bool {
+	for _, haves := range n.Haves {
+		if h, ok := haves[module]; ok && h.Health == delivery.HealthFailed {
+			return true
+		}
+	}
+	return false
+}
+
 // neitherOne is a module assigned to a node that holds no want for it after a pass, and the rule holding its first want back.
 type neitherOne struct {
 	node, module string
@@ -458,7 +458,7 @@ func TestThePassGathersTheModulesOnNeitherList(t *testing.T) {
 		{nil, ""},
 	} {
 		conds = nil
-		p.sayNodeFailed("rofi", fpTest, []string{"shanks"}, assign.Outcome{NodeFailed: c.failed}, false)
+		p.sayNodeFailed("rofi", fpTest, []string{"shanks"}, assign.Nodes{}, assign.Outcome{NodeFailed: c.failed}, false)
 		switch {
 		case c.state == "" && len(conds) != 0, c.state != "" && (len(conds) != 1 || conds[0].State != c.state):
 			t.Errorf("node failed, pass %d said %+v, want %q", i, conds, c.state)
@@ -467,6 +467,40 @@ func TestThePassGathersTheModulesOnNeitherList(t *testing.T) {
 			t.Errorf("node failed, raised %+v", conds[0])
 		}
 	}
+	// D2: a node that leaves the module's nodes while it failed alone is cleared once; the first pass after a start clears every
+	// node the presence bucket knows, for a module with a failed have, and nothing for one without.
+	conds = nil
+	p.sayNodeFailed("rofi", fpTest, []string{"shanks"}, assign.Nodes{}, assign.Outcome{NodeFailed: failing}, false)
+	p.sayNodeFailed("rofi", fpTest, nil, assign.Nodes{}, assign.Outcome{}, false)
+	p.sayNodeFailed("rofi", fpTest, nil, assign.Nodes{}, assign.Outcome{}, false)
+	if len(conds) != 2 || conds[1].State != asks.StateCleared || conds[1].Key != "mesh-delivery-assigner.failed-alone.shanks.rofi" {
+		t.Errorf("a node leaving the module said %+v, want one raise and one clear", conds)
+	}
+	known := assign.Nodes{Online: map[string]bool{"ace": true, "g14": false},
+		Haves: map[string]map[string]delivery.Have{"g14": {"rofi": {FP: fpTest, Health: delivery.HealthFailed}}}}
+	for _, c := range []struct {
+		name  string
+		known assign.Nodes
+		keys  []string
+	}{
+		{"a failed have", known, []string{"mesh-delivery-assigner.failed-alone.ace.rofi", "mesh-delivery-assigner.failed-alone.g14.rofi"}},
+		{"no failed have", assign.Nodes{Online: known.Online}, nil},
+	} {
+		conds = nil
+		fresh := newPasser(buckets{}, time.Date(2026, 10, 26, 22, 0, 0, 0, time.UTC))
+		fresh.logf = func(string, ...any) {}
+		fresh.say = func(c asks.Condition) error { conds = append(conds, c); return nil }
+		fresh.sayNodeFailed("rofi", fpTest, nil, c.known, assign.Outcome{}, true)
+		var keys []string
+		for _, cond := range conds {
+			if cond.State == asks.StateCleared {
+				keys = append(keys, cond.Key)
+			}
+		}
+		if !slices.Equal(keys, c.keys) || len(conds) != len(c.keys) {
+			t.Errorf("first pass, %s: said %+v, want clears of %q", c.name, conds, c.keys)
+		}
+	}
 }

 // A neither or held condition a previous process raised is cleared by the first pass after a start: a fresh pass remembers
@@ -235,11 +235,12 @@ func Wants(m Module, n Nodes, wants map[string]delivery.Want, lastHealthy map[st
 			if !ok || h.FP != m.FP || !failed(h) {
 				continue
 			}
-			if others := healthyElsewhere(m, n, node); len(others) > 0 {
+			if others := healthyElsewhere(m, n, node); len(others) > 0 && !failedElsewhere(m, n, node) {
 				// F runs healthy on another node, so its failure here is this node's, not F's: F is not held, no node is set back,
 				// and the failing node keeps its want (ADR 0557 §1; issue 1496: on ace a newly assigned mesh-build-node failed its
 				// first check and the hold moved shanks, g14 and novox, healthy on F for an hour, back). The pass raises the node's
-				// condition. With no healthy witness the hold and the set-back stand (ADR 0557 §2, ADR 0429 §6).
+				// condition. With no online healthy witness, or with F failed on a second node, the hold and the set-back stand (ADR 0557
+				// §2, ADR 0429 §6).
 				if out.NodeFailed == nil {
 					out.NodeFailed = map[string]NodeFailure{}
 				}
@@ -435,17 +436,30 @@ func failed(h delivery.Have) bool {
 	return h.Health == delivery.HealthFailed && (h.Phase == delivery.PhaseRunning || h.Phase == delivery.PhaseFailed)
 }

-// healthyElsewhere is every node of m but node that runs m.FP healthy, by name.
+// healthyElsewhere is every online node of m but node that runs m.FP healthy, by name.
 func healthyElsewhere(m Module, n Nodes, node string) []string {
 	var out []string
 	for _, other := range slices.Sorted(slices.Values(m.Nodes)) {
-		if other != node && healthy(n.Haves[other][m.Name], m.FP) {
+		// A witness is online: a node that sleeps keeps the have it last wrote, and a stale have is no evidence that F runs
+		// healthy now.
+		if other != node && n.Online[other] && healthy(n.Haves[other][m.Name], m.FP) {
 			out = append(out, other)
 		}
 	}
 	return out
 }

+// failedElsewhere says whether another node of m has a have naming m.FP failed: then the failure is not one node's alone, and
+// the hold and the set-back stand at the second failure (ADR 0557 §2).
+func failedElsewhere(m Module, n Nodes, node string) bool {
+	for _, other := range m.Nodes {
+		if h, ok := n.Haves[other][m.Name]; ok && other != node && h.FP == m.FP && failed(h) {
+			return true
+		}
+	}
+	return false
+}
+
 // NodeFailure is a failed health check of a module's newest fingerprint on one node while it runs healthy on Others: the have's
 // health, phase and words (HeldWords without a time), and the healthy witnesses by name.
 type NodeFailure struct {
@@ -163,6 +163,33 @@ func TestAFailedHealthHoldsAndSetsBack(t *testing.T) {
 		len(out.NodeFailed) != 1 {
 		t.Errorf("node failed = %+v", out.NodeFailed)
 	}
+
+	// ADR 0557 §2: a witness that is offline keeps the have it last wrote and witnesses nothing (D1); F failing on a second node is
+	// not one node's failure (D3). Either way F is held and ace is set back.
+	three := m
+	three.Nodes = []string{"ace", "g14", "novox"}
+	for _, c := range []struct {
+		name   string
+		online map[string]bool
+		novox  delivery.Health
+	}{
+		{"the witness sleeps", map[string]bool{"ace": true, "g14": false, "novox": true}, delivery.HealthOK},
+		{"F fails on a second node", map[string]bool{"ace": true, "g14": true, "novox": true}, delivery.HealthFailed},
+	} {
+		n.Online = c.online
+		n.Haves["novox"] = map[string]delivery.Have{"mail": {FP: fNew, Phase: delivery.PhaseRunning, Health: c.novox}}
+		if c.novox == delivery.HealthOK {
+			n.Haves["novox"]["mail"] = delivery.Have{FP: fOld, Phase: delivery.PhaseRunning, Health: delivery.HealthOK}
+		}
+		before["novox"] = delivery.Want{FP: fNew, Composed: dig(fNew)}
+		out = Wants(three, n, before, map[string]string{"ace": fOld, "g14": fOld, "novox": fOld})
+		if out.Held == nil || out.Held.Node != "ace" || len(out.NodeFailed) != 0 {
+			t.Errorf("%s: held %+v, node failed %+v: want F held at ace", c.name, out.Held, out.NodeFailed)
+		}
+		if w := out.Wants["ace"]; w.FP != fOld {
+			t.Errorf("%s: ace's want = %+v, want set back", c.name, w)
+		}
+	}
 }

 // 1297/B4: a module holding data is not set back; its want stays and says why.