A reconcile applies what was kept when its turn comes, not when its timer fired (hq issues 257, 261)

The five-minute reconcile read the kept declaration and then waited for the link's apply; the
link keeps a declaration only once its apply ends, so the reconcile applied the older one over
it. A module assigned a moment earlier was given back, and the report named a declaration the
mesh no longer recorded as sent, which held a plan at its first machine.

cmd/mesh-host/main.go

@@ -8,6 +8,7 @@ package main
 
 import (
 	"context"
+	"crypto/ed25519"
 	"crypto/sha256"
 	"encoding/base64"
 	"encoding/hex"
@@ -1254,7 +1255,7 @@ func holdTheMachine(ctx context.Context, opts options, mine identity.Identity, s
 		case <-ticker.C:
 		}
 
-		declared, err := store.LoadDeclared(store.DeclaredPath(opts.state), mine.Membership.Signer)
+		report, err := reapplyKept(ctx, opts, mine.Membership.Signer, sched, say)
 		if errors.Is(err, store.ErrNothingDeclared) {
 			// Nothing to hold this machine to yet. Ordinary on a node that has enrolled and not
 			// been assigned anything.
@@ -1264,8 +1265,6 @@ func holdTheMachine(ctx context.Context, opts options, mine identity.Identity, s
 			say("cannot re-apply what this node was told: " + err.Error())
 			continue
 		}
-
-		report := applyDeclared(ctx, opts, declared, sched, say)
 		// A reconcile is otherwise silent. On an adopted node it speaks when what it holds or
 		// its firewall changed, because that is how a predecessor still writing is caught
 		// (novox/hq ADR 0100); publish decides whether anything did.
@@ -1309,15 +1308,6 @@ func worthSaying(report link.Report) bool {
 		len(report.Filters) > 0 || report.FoundFirewall != nil || len(report.Windows) > 0
 }
 
-// applyDeclared applies a declaration that has already been proved to come from the mesh.
-//
-// Signature checking happens before this is called, in the link. By the time anything here runs,
-// the question "is this from the mesh I joined" is settled — which is why this can treat the
-// bytes as instructions.
-func applyDeclared(ctx context.Context, opts options, raw []byte, sched *apply.Scheduler, say link.Announce) link.Report {
-	return applyAndKeep(ctx, opts, raw, nil, sched, say)
-}
-
 // announceOr is what the apply writes its detail with, given what the caller has to say things with.
 //
 // **Never nil.** This argument was nil on the serving path, and nil is silence: everything the apply
@@ -1351,6 +1341,31 @@ func applyAndKeep(ctx context.Context, opts options, raw []byte, signed *store.D
 	sched *apply.Scheduler, say link.Announce) link.Report {
 	applying.Lock()
 	defer applying.Unlock()
+	return applyAndKeepHeld(ctx, opts, raw, signed, sched, say)
+}
+
+// reapplyKept re-applies what this node was last told, **read once it is this apply's turn**.
+//
+// Read before waiting, it was whatever had been kept when the timer fired — and a declaration
+// the link was applying at that moment is kept only once its apply ends. The reconcile then waited
+// for that apply and applied the older declaration over it: a module assigned a second earlier was
+// given back on the spot, and the report it sent named a declaration the mesh no longer recorded
+// as sent, so a plan waiting on this machine waited for a report that could not come (novox/hq
+// issues 257, 261). Read under the lock, it is always the latest the mesh said.
+func reapplyKept(ctx context.Context, opts options, signer ed25519.PublicKey,
+	sched *apply.Scheduler, say link.Announce) (link.Report, error) {
+	applying.Lock()
+	defer applying.Unlock()
+	declared, err := store.LoadDeclared(store.DeclaredPath(opts.state), signer)
+	if err != nil {
+		return link.Report{}, err
+	}
+	return applyAndKeepHeld(ctx, opts, declared, nil, sched, say), nil
+}
+
+// applyAndKeepHeld is applyAndKeep for a caller already holding applying.
+func applyAndKeepHeld(ctx context.Context, opts options, raw []byte, signed *store.Declared,
+	sched *apply.Scheduler, say link.Announce) link.Report {
 	unlock, err := store.Lock(opts.state, nil)
 	if err != nil {
 		return link.Report{Refused: err.Error()}

cmd/mesh-host/main_test.go

@@ -609,3 +609,63 @@ func TestTheApplysLogIsNeverNil(t *testing.T) {
 		t.Fatalf("the apply's detail did not reach the caller's announce: %v", said)
 	}
 }
+
+// Defends novox/hq issues 257 and 261: a reconcile due while the link applies a newer declaration
+// applies that newer one once its turn comes — never the one kept when its timer fired. Read before
+// waiting, it gave back a module assigned a second earlier, and reported a declaration the mesh no
+// longer recorded as sent.
+func TestAReconcileAppliesWhatWasKeptWhenItsTurnComes(t *testing.T) {
+	built, err := system.For("arch")
+	if err != nil || built.Confirm(context.Background(), apply.ExecRunner) != nil {
+		t.Skip("this machine is not one these tests can apply on")
+	}
+	was := builtFor
+	builtFor = "arch"
+	t.Cleanup(func() { builtFor = was })
+	dir := t.TempDir()
+	opts := options{state: filepath.Join(dir, "state.json")}
+	public, private, err := ed25519.GenerateKey(nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	target := filepath.Join(dir, "a.conf")
+	kept := func(content string) store.Declared {
+		body := []byte(`{"declaration":1,"resources":[{"id":"a","type":"file","path":"` + target +
+			`","content":"` + content + `\n"}]}`)
+		return store.Declared{Declaration: body, Signature: ed25519.Sign(private, body)}
+	}
+	if err := store.SaveDeclared(store.DeclaredPath(opts.state), kept("older")); err != nil {
+		t.Fatal(err)
+	}
+
+	// The link is applying: the reconcile's timer fires and it waits its turn.
+	applying.Lock()
+	done := make(chan error, 1)
+	go func() {
+		_, err := reapplyKept(context.Background(), opts, public, nil, nil)
+		done <- err
+	}()
+	time.Sleep(50 * time.Millisecond)
+	// The link's apply ends by keeping the newer declaration, and lets go.
+	if err := store.SaveDeclared(store.DeclaredPath(opts.state), kept("newer")); err != nil {
+		applying.Unlock()
+		t.Fatal(err)
+	}
+	applying.Unlock()
+
+	select {
+	case err := <-done:
+		if err != nil {
+			t.Fatal(err)
+		}
+	case <-time.After(10 * time.Second):
+		t.Fatal("the reconcile never ran once the node was free")
+	}
+	got, err := os.ReadFile(target)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if string(got) != "newer\n" {
+		t.Errorf("the reconcile applied %q, what was kept before the newer declaration", got)
+	}
+}