diff --git a/internal/inventory/sends.go b/internal/inventory/sends.go index fd13eda3..edba4ac8 100644 --- a/internal/inventory/sends.go +++ b/internal/inventory/sends.go @@ -244,15 +244,21 @@ func (i *Inventory) LastSends(ctx context.Context) (map[string]Send, error) { } // lastSends reads the newest recorded send of each machine matching where, beside the machine's last report -// when that report is about the send: the same declaration (its digest), reported after it was sent — both -// times the store's own, so no machine's clock decides it — so a machine sent back a declaration it once -// applied does not read as having applied the new send before it said so (novox/hq issue 485). +// when that report is about the send (novox/hq issue 485). It names the send's declaration (its digest), and: +// - from an ordered node-engine, one that reports the epoch and sequence it acted on, those are the send's +// own. A periodic report about an identical earlier send names the same digest and may arrive after a later +// send the machine never received; only the sequence tells them apart. It also holds for a fast machine +// whose report lands before the send's row is stamped. +// - from an engine that reports no order, the report came after the send — both times the store's own — so +// a machine sent again a declaration it once applied does not read as having applied the new send. func (i *Inventory) lastSends(ctx context.Context, where string, args ...any) (map[string]Send, error) { rows, err := i.store.Pool().Query(ctx, `select distinct on (n.name) `+sendColumns+`, coalesce(r.outcome, ''), coalesce(r.refused, ''), case when jsonb_typeof(r.failed) = 'array' then jsonb_array_length(r.failed) else 0 end, r.at from declaration_send s join node n on n.id = s.node left join node_report r on r.node = s.node and r.declared <> '' and r.declared = s.digest - and r.at >= s.sent_at + and case when r.reported_sequence is not null + then r.reported_sequence = s.sequence and coalesce(r.reported_epoch, 0) = coalesce(s.epoch, 0) + else r.at >= s.sent_at end where s.recorded and `+where+` order by n.name, s.id desc`, args...) if err != nil { diff --git a/internal/inventory/sends_test.go b/internal/inventory/sends_test.go index 6b70fdac..37aaafd6 100644 --- a/internal/inventory/sends_test.go +++ b/internal/inventory/sends_test.go @@ -280,3 +280,69 @@ func TestTheLastSendSaysWhatTheMachineAnswered(t *testing.T) { t.Error("a machine never sent anything has a last send") } } + +// **A report about another declaration, or another send of the same one, is not the last send's answer** +// (novox/hq issue 485, review of mesh-controller #234). A report naming an earlier send's digest that arrives +// after a later send is not the later send's answer. And from an ordered node-engine, a periodic report about +// an identical earlier send — the same digest, arriving after the later send — is not its answer either: only +// the report's epoch and sequence say which send it is about. +func TestAReportIsTheLastSendsAnswerOnlyWhenItIsAboutThatSend(t *testing.T) { + inv, node := aNodeWithModules(t) + ctx := t.Context() + record, err := inv.NodeByName(ctx, node) + if err != nil { + t.Fatal(err) + } + send := func(sequence int64, digest string) { + t.Helper() + if err := inv.RecordSentWith(ctx, record.ID, digest, nil, 57, 40, Send{Sequence: sequence, Epoch: 57, + Sender: "the controller's daemon (pid 7 on anchor)", Generation: 40, Digest: digest}); err != nil { + t.Fatal(err) + } + } + ordered := func(epoch, sequence int64, digest string) { + t.Helper() + if _, err := inv.RecordOrderedDoing(ctx, record.ID, Doing{Outcome: OutcomeApplied, Declared: digest, Applied: 1}, + ReportOrder{Epoch: epoch, Sequence: sequence}, func(ReportOrder) bool { return false }); err != nil { + t.Fatal(err) + } + } + answer := func() string { + t.Helper() + last, found, err := inv.LastSend(ctx, node) + if err != nil || !found { + t.Fatalf("the last send is not read back: %v, %v", found, err) + } + return last.Answer.Outcome + } + + // An engine that reports no order: the digest decides. + send(11, "d11") + send(12, "d12") + if _, err := inv.RecordDoing(ctx, record.ID, Doing{Outcome: OutcomeApplied, Declared: "d11"}); err != nil { + t.Fatal(err) + } + if a := answer(); a != "" { + t.Fatalf("a report about d11, after d12 was sent, reads as d12's answer: %q", a) + } + + // An ordered engine: the epoch and sequence decide, whatever the digest and the time. + send(20, "d20") + ordered(57, 20, "d20") + if a := answer(); a != OutcomeApplied { + t.Fatalf("an ordered report about sequence 20 is not its answer: %q", a) + } + send(21, "d20") + ordered(57, 20, "d20") + if a := answer(); a != "" { + t.Fatalf("a periodic report about sequence 20, after 21 was sent with the same digest, reads as 21's answer: %q", a) + } + ordered(56, 21, "d20") + if a := answer(); a != "" { + t.Fatalf("a report about sequence 21 of another epoch reads as this send's answer: %q", a) + } + ordered(57, 21, "d20") + if a := answer(); a != OutcomeApplied { + t.Fatalf("an ordered report about sequence 21 is not its answer: %q", a) + } +} diff --git a/testdata/store-tests b/testdata/store-tests index 0b574af4..97f856c8 100644 --- a/testdata/store-tests +++ b/testdata/store-tests @@ -2,4 +2,5 @@ # each one named here, by package, and fails unless every one passed — a test that needs the store skips without # one, and a skip passes unseen. One per line: . Lines starting with # are said nothing of. ./internal/inventory TestTheLastSendSaysWhatTheMachineAnswered +./internal/inventory TestAReportIsTheLastSendsAnswerOnlyWhenItIsAboutThatSend ./cmd/mesh-controller TestNodeShowAndStatusReadTheLastSendFromTheStore