Keep a no-holder machine as ADR 0404 says; prove the cut takes the backups before its first tier (issue 1086, review round 1)

cmd/mesh-controller/backups_before_send_integration_test.go

@@ -4,6 +4,7 @@ package main
 
 import (
 	"encoding/json"
+	"os"
 	"strings"
 	"sync"
 	"testing"
@@ -217,3 +218,91 @@ func TestADeliveryThatMovesABackedUpModuleWaitsForItsBackupAndAFailedOneHoldsItA
 		t.Fatalf("the condition stays: %+v", said)
 	}
 }
+
+// Issue 1086: a plan cut from a merge asked its first tier at the cut, so a module keeping data was sent without the
+// backup the plan promised. The cut now takes the backups first (askFirstTierAtCut, batches.go), and the walk's own
+// step asks the tier once they are done.
+func TestAPlanCutFromAMergeOfABackedUpModuleAsksItsBackupAndNotItsTierUntilItIsDone(t *testing.T) {
+	open := aMesh(t)
+	ctx := t.Context()
+	inv := open.inventory
+	settings := t.TempDir() + "/merge-window.json"
+	if err := os.WriteFile(settings, []byte(`{"merge-window": "90s", "merge-window-at-most": "10m"}`), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	t.Setenv(mergeWindowFileVar, settings)
+	everyHeadWaits(t)
+	asked := asksRecorded(t)
+	js, err := broker.Dial(testbus.URL(t))
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(js.Close)
+	before := handActConn
+	handActConn = js.Conn()
+	t.Cleanup(func() { handActConn = before })
+
+	if _, err := inv.SeedSeats(ctx, catalogue.DefaultSeats()); err != nil {
+		t.Fatal(err)
+	}
+	register(t, open, catalogue.Manifest{Module: "keeper", Version: "1",
+		Claims: []catalogue.Claim{{Name: catalogue.BackupSeat, Scope: catalogue.ScopeNode, Serves: []string{"backed-up", "now", "restore"}}}})
+	if err := inv.RegisterModule(ctx, catalogue.Manifest{Module: "mesh-controller", Version: "1"},
+		inventory.Source{Repository: "novox/mesh-controller", Seat: "git", Ref: "main", BuiltFrom: "c0", Head: "c0"}); err != nil {
+		t.Fatal(err)
+	}
+	notes, err := catalogue.ParseManifest([]byte(`{"module":"notes","version":"1","data":{"own":[` +
+		`{"id":"files","path":"${dir:files}","class":"valuable","why":"people's notes"}]},` +
+		`"resources":[{"id":"files","type":"directory"}]}`))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := inv.RegisterModule(ctx, notes, inventory.Source{Repository: "novox/notes", Seat: "git", Ref: "main",
+		BuiltFrom: "c0", Head: "c0"}); err != nil {
+		t.Fatal(err)
+	}
+	for _, m := range []string{"keeper", "notes"} {
+		if _, err := assign(ctx, open, "anchor", m); err != nil {
+			t.Fatal(err)
+		}
+	}
+	seat := &aBackupSeatOnTheBus{}
+	seat.serve(t, js.Conn(), "anchor")
+	wasWithin, wasEvery := backupsWithin, backupsPollEvery
+	backupsWithin, backupsPollEvery = time.Minute, 10*time.Millisecond
+	t.Cleanup(func() { backupsWithin, backupsPollEvery = wasWithin, wasEvery })
+
+	// 1. A merge of notes is heard and its window closes: the batch is cut through cutBatch.
+	hear(t, open, repoMerge("notes", "c-notes", t0), t0.Add(time.Second))
+	cutAt(t, open, t0.Add(time.Second+90*time.Second))
+	ws, _ := walks(t, open)
+	if len(ws) != 1 {
+		t.Fatalf("%d walk(s)", len(ws))
+	}
+	// 2. Right after the cut no tier is asked, the record has anchor/notes pending, the seat heard one `now`.
+	cut, err := inv.PlanByID(ctx, ws[0].ID)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(*asked) != 0 || askedInTier(cut) {
+		t.Fatalf("the first tier was asked at the cut: %v %+v", *asked, cut.Modules)
+	}
+	if cut.Backups == nil || cut.Backups.State != inventory.BackupPending || len(cut.Backups.Asked) != 1 ||
+		cut.Backups.Asked[0].Machine != "anchor" || cut.Backups.Asked[0].Module != "notes" {
+		t.Fatalf("the backups of the cut: %+v", cut.Backups)
+	}
+	if seat.asks() != 1 {
+		t.Fatalf("the seat heard %d `now`", seat.asks())
+	}
+	// 3. The backup answers done: the walk's own step asks the first tier, once, and asks no second backup.
+	seat.answers("notes", true, "")
+	advancePlans(ctx, open)
+	advancePlans(ctx, open)
+	after, err := inv.PlanByID(ctx, ws[0].ID)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(*asked) != 1 || after.Backups.State != inventory.BackupDone || seat.asks() != 1 {
+		t.Fatalf("builds=%v backups=%+v asks=%d", *asked, after.Backups, seat.asks())
+	}
+}

cmd/mesh-controller/plan_backups.go

@@ -240,7 +240,7 @@ func stepBackupsOnce(ctx context.Context, p *inventory.Plan, facts backupFacts,
 }
 
 // wantedBackups is the record of backups a plan has to take, or nil for none: one entry per machine with a holder
-// running a moved module that has a backed-up item; a machine with no holder is a failed entry that holds the plan.
+// running a moved module that has a backed-up item, and the machines with none.
 func wantedBackups(p inventory.Plan, facts backupFacts) *inventory.PlanBackups {
 	var asked []inventory.PlanBackup
 	noHolder := map[string]bool{}
@@ -248,12 +248,7 @@ func wantedBackups(p inventory.Plan, facts backupFacts) *inventory.PlanBackups {
 		for _, module := range tier {
 			for _, machine := range facts.backedUp[module] {
 				if !facts.holder[machine] {
-					// A machine the plan sends data to and cannot ask holds the plan (issue 1086): a send never
-					// goes on without the restore point it promised.
 					noHolder[machine] = true
-					asked = append(asked, inventory.PlanBackup{Machine: machine, Module: module,
-						State: inventory.BackupFailed, Words: "could not be asked: " + machine + " holds no " +
-							catalogue.BackupSeat})
 					continue
 				}
 				asked = append(asked, inventory.PlanBackup{Machine: machine, Module: module, State: inventory.BackupPending})

cmd/mesh-controller/plan_backups_test.go

@@ -111,8 +111,8 @@ func TestOnlyModulesWithAnItemProtectedByABackupAreAsked(t *testing.T) {
 	}
 }
 
-// A machine with no backup holder cannot be asked: the plan holds and says so, and asks nothing (issue 1086).
+// A machine with no backup holder is asked nothing, the plan says so, and it does not hold the plan.
-func TestANodeWithNoBackupHolderHoldsThePlanAndSaysSo(t *testing.T) {
+func TestANodeWithNoBackupHolderAsksNothingAndThePlanSaysSo(t *testing.T) {
 	seat := &aFakeBackupSeat{}
 	seat.install(t)
 	entries := []inventory.Entry{aModuleWith(t, "copied", []string{"ace", "bare"}, anItem("a", ""))}
@@ -121,17 +121,17 @@ func TestANodeWithNoBackupHolderHoldsThePlanAndSaysSo(t *testing.T) {
 	if err != nil || proceed {
 		t.Fatalf("proceed=%v err=%v", proceed, err)
 	}
-	if len(seat.asked) != 0 {
+	if strings.Join(seat.asked, " ") != "ace/copied" {
-		t.Fatalf("a held plan asks nothing: %v", seat.asked)
+		t.Fatalf("asked %v", seat.asked)
 	}
-	if p.Backups == nil || len(p.Backups.NoHolder) != 1 || p.Backups.NoHolder[0] != "bare" ||
+	if p.Backups == nil || len(p.Backups.NoHolder) != 1 || p.Backups.NoHolder[0] != "bare" {
-		p.Backups.State != inventory.BackupFailed || !strings.Contains(p.Note, "copied on bare") {
+		t.Fatalf("the plan must say bare has no holder: %+v", p.Backups)
-		t.Fatalf("the plan must hold and say bare has no holder: %+v note %q", p.Backups, p.Note)
 	}
+	// Only the no-holder machine: nothing to wait for, the plan goes on, and still says it.
 	only := []inventory.Entry{aModuleWith(t, "copied", []string{"bare"}, anItem("a", ""))}
 	q := aPlanOf("copied")
 	proceed, err = stepBackups(t.Context(), q, backupFactsFrom(only, holders("ace")), backupT0)
-	if err != nil || proceed || q.Backups.State != inventory.BackupFailed {
+	if err != nil || !proceed || q.Backups == nil || q.Backups.NoHolder[0] != "bare" || q.Backups.State != inventory.BackupDone {
 		t.Fatalf("proceed=%v err=%v record=%+v", proceed, err, q.Backups)
 	}
 }
@@ -264,24 +264,6 @@ func TestACutPlanAsksTheBackupOfGiteaOnNovoxBeforeItAsksItsFirstTier(t *testing.
 	}
 }
 
-func TestACutPlanHoldsWhenTheMachineItSendsDataToHasNoBackupHolder(t *testing.T) {
-	seat := &aFakeBackupSeat{}
-	seat.install(t)
-	facts := backupFactsFrom([]inventory.Entry{giteaOnNovox(t)}, holders("ace"))
-	p := aPlanOf("gitea")
-	tierAsked := false
-	err := askFirstTierAtCut(p,
-		func(p *inventory.Plan) (bool, error) { return stepBackups(t.Context(), p, facts, backupT0) },
-		func() error { tierAsked = true; return nil })
-	if err != nil || tierAsked {
-		t.Fatalf("asked=%v err=%v", tierAsked, err)
-	}
-	if p.Backups == nil || p.Backups.State != inventory.BackupFailed || !strings.Contains(p.Note, "novox") ||
-		!strings.Contains(p.Note, "held") {
-		t.Fatalf("the plan must hold and say so: %+v note %q", p.Backups, p.Note)
-	}
-}
-
 func TestACutPlanWhoseBackupsCouldNotBeWorkedOutAsksNothingAndSaysSo(t *testing.T) {
 	p := aPlanOf("gitea")
 	tierAsked := false