Merge pull request 'Keep the account of the sent declaration over an older one (hq issue 267)' (#74) from fix/stale-report-overwrites into main

internal/inventory/nodes.go

@@ -851,9 +851,9 @@ func (i *Inventory) DoingOf(ctx context.Context, name string) (Doing, bool, erro
 	var d Doing
 	var failed []byte
 	err = i.store.Pool().QueryRow(ctx,
-		`select outcome, refused, failed, applied, at, failing_since, failures
+		`select outcome, refused, failed, applied, at, failing_since, failures, coalesce(declared, '')
 		 from node_report where node = $1`, node.ID).
-		Scan(&d.Outcome, &d.Refused, &failed, &d.Applied, &d.At, &d.Since, &d.Times)
+		Scan(&d.Outcome, &d.Refused, &failed, &d.Applied, &d.At, &d.Since, &d.Times, &d.Declared)
 	if errors.Is(err, pgx.ErrNoRows) {
 		return Doing{}, false, nil
 	}

internal/link/enrolment.go

@@ -410,6 +410,25 @@ func (e Enrolment) Heard(ctx context.Context, report Report) (news bool, err err
 	if err := e.Inventory.RecordCarried(ctx, report.Node, report.Carried); err != nil {
 		return false, err
 	}
+	// **An account of a declaration the mesh has moved past never replaces the account it keeps**
+	// (novox/hq issue 267). The machine-facts half of such a report is kept above, whenever it
+	// arrives; this half is the machine's word on what it did with what it was sent, and the release
+	// plan reads it as such. A reconcile that held a machine to the older declaration a moment before
+	// the newer one arrived reported *after* the newer apply's report, and stored last, it read as the
+	// machine never having applied what it was sent — the plan waited until somebody pushed by hand.
+	// One row per node means the last write wins, so the order of arrival must not decide it.
+	if report.Declared != "" {
+		sent, err := e.Inventory.Outstanding(ctx, report.Node)
+		if err != nil {
+			return false, err
+		}
+		if Superseded(report.Declared, sent) {
+			log.Printf("kept what %s says about the machine, and not its account of declaration %s: "+
+				"the mesh has sent it %s since", report.Node, short(report.Declared), short(sent))
+			return false, e.Inventory.Seen(ctx, node.ID)
+		}
+	}
+
 	// **Whether this is news** is the store's answer: it holds the previous report, and a machine
 	// that reconciles every minute says the same thing until something changes (novox/hq ADR 0134).
 	news, err = e.Inventory.RecordDoing(ctx, node.ID, doing)

internal/link/heard_test.go

@@ -260,3 +260,67 @@ func TestWhatFiltersAMachineIsKeptFromItsReport(t *testing.T) {
 		t.Fatalf("the next report did not replace what filters the machine: %+v", again)
 	}
 }
+
+// Measured on the home server (novox/hq issue 267): the mesh sent d2; the machine applied it and
+// reported, and a reconcile's report about d1 — made just before d2 arrived — reached the mesh after
+// it. Stored last, it read as the machine never having applied d2, and the release plan waited on a
+// report it had already been given.
+func TestAnAccountOfAnOlderDeclarationDoesNotReplaceTheNewer(t *testing.T) {
+	inv := inventory.ForTest(t)
+	ctx := context.Background()
+	node, err := inv.AddNode(ctx, "home-server")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := inv.RecordSent(ctx, node.ID, "d2", nil); err != nil {
+		t.Fatal(err)
+	}
+	heard := link.Enrolment{Inventory: inv}
+	if _, err := heard.Heard(ctx, link.Report{Node: "home-server", Declared: "d2",
+		Applied: []string{"a", "b"}, Outward: []string{"eth0"}}); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := heard.Heard(ctx, link.Report{Node: "home-server", Declared: "d1",
+		Applied: []string{"a"}, Outward: []string{"eth1"}}); err != nil {
+		t.Fatal(err)
+	}
+	doing, said, err := inv.DoingOf(ctx, "home-server")
+	if err != nil || !said {
+		t.Fatalf("no account kept: %v", err)
+	}
+	if doing.Declared != "d2" || doing.Applied != 2 {
+		t.Fatalf("the older report replaced the account of what was sent: %+v", doing)
+	}
+	// What it says about the machine is kept whenever it arrives, as before.
+	if links, err := inv.OutwardLinksOf(ctx, "home-server"); err != nil || len(links) != 1 || links[0] != "eth1" {
+		t.Fatalf("what the older report said about the machine was not kept: %v %v", links, err)
+	}
+}
+
+// The account of the declaration that is outstanding replaces whatever was kept, and so does one
+// from a machine nothing was ever recorded as sent to.
+func TestAnAccountOfTheSentDeclarationReplacesTheKeptOne(t *testing.T) {
+	inv := inventory.ForTest(t)
+	ctx := context.Background()
+	node, err := inv.AddNode(ctx, "home-server")
+	if err != nil {
+		t.Fatal(err)
+	}
+	heard := link.Enrolment{Inventory: inv}
+	if _, err := heard.Heard(ctx, link.Report{Node: "home-server", Declared: "d1", Applied: []string{"a"}}); err != nil {
+		t.Fatal(err)
+	}
+	if err := inv.RecordSent(ctx, node.ID, "d2", nil); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := heard.Heard(ctx, link.Report{Node: "home-server", Declared: "d2", Applied: []string{"a", "b"}}); err != nil {
+		t.Fatal(err)
+	}
+	doing, _, err := inv.DoingOf(ctx, "home-server")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if doing.Declared != "d2" || doing.Applied != 2 {
+		t.Fatalf("the account of what was sent was not kept: %+v", doing)
+	}
+}