From b6f8aef4d4f11b86ea82cbee92149c688385b906 Mon Sep 17 00:00:00 2001 From: jochen Date: Sun, 18 Oct 2026 01:57:00 +0200 Subject: [PATCH] Keep a no-holder machine as ADR 0404 says; prove the cut takes the backups before its first tier (issue 1086, review round 1) --- .../backups_before_send_integration_test.go | 89 +++++++++++++++++++ cmd/mesh-controller/plan_backups.go | 7 +- cmd/mesh-controller/plan_backups_test.go | 34 ++----- 3 files changed, 98 insertions(+), 32 deletions(-) diff --git a/cmd/mesh-controller/backups_before_send_integration_test.go b/cmd/mesh-controller/backups_before_send_integration_test.go index 58f89ed0..10da5daa 100644 --- a/cmd/mesh-controller/backups_before_send_integration_test.go +++ b/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()) + } +} diff --git a/cmd/mesh-controller/plan_backups.go b/cmd/mesh-controller/plan_backups.go index 32377e19..f741e126 100644 --- a/cmd/mesh-controller/plan_backups.go +++ b/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}) diff --git a/cmd/mesh-controller/plan_backups_test.go b/cmd/mesh-controller/plan_backups_test.go index 941fbda7..522611ee 100644 --- a/cmd/mesh-controller/plan_backups_test.go +++ b/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). -func TestANodeWithNoBackupHolderHoldsThePlanAndSaysSo(t *testing.T) { +// A machine with no backup holder is asked nothing, the plan says so, and it does not hold the plan. +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 { - t.Fatalf("a held plan asks nothing: %v", seat.asked) + if strings.Join(seat.asked, " ") != "ace/copied" { + t.Fatalf("asked %v", seat.asked) } - 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 hold and say bare has no holder: %+v note %q", p.Backups, p.Note) + if p.Backups == nil || len(p.Backups.NoHolder) != 1 || p.Backups.NoHolder[0] != "bare" { + t.Fatalf("the plan must say bare has no holder: %+v", p.Backups) } + // 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