carry dropped fact records through the failure path and count them

Report the accumulated warnings on stderr when the estate load fails, and fold
the dropped-record tally into the error. Count records rather than warning
messages in --explain, by carrying the count on the Warn hook.

README.md

@@ -245,8 +245,10 @@ A missed host means an unvalidated catalog, so every ambiguity widens:
 A fact record that arrives with no certname belongs to no host, and one whose
 value does not decode carries nothing to fold; either way it cannot enter the
 estate table, and a host whose records all fail leaves the table with them. Both
-are reported in `node_warnings`, in `--explain` and on stderr, and set
+are reported in `node_warnings`, in `--explain` and on stderr, counted in
-`summary.resolution_incomplete` too.
+`summary.dropped_fact_records`, and set `summary.resolution_incomplete` too. If
+the drops empty the table altogether the run fails, and the warnings and the
+tally go to stderr with the error rather than dying with it.
 
 Paths outside the Puppet modulepath — `.woodpecker/`, `doc/`, `spec/`,
 `vendor/`, `Gemfile`, `*.md` — cannot reach a catalog and are skipped.

cmd/catalog-diff/main.go

@@ -254,7 +254,7 @@ func run(ctx context.Context, out io.Writer, hostname string, opts *options) err
 
 	server := puppetserver.New(opts.server, tlsCfg, opts.timeout)
 	facts := puppetdb.New(opts.puppetdb, opts.timeout)
-	facts.Warn = func(msg string) { fmt.Fprintln(os.Stderr, "warning:", msg) }
+	facts.Warn = func(msg string, _ int) { fmt.Fprintln(os.Stderr, "warning:", msg) }
 
 	healthCtx, cancel := context.WithTimeout(ctx, 30*time.Second)
 	defer cancel()

cmd/catalog-impact/main.go

@@ -120,18 +120,21 @@ func run(ctx context.Context, out, errOut io.Writer, opts *options) error {
 	}
 
 	// A dropped fact record can take a whole host out of the estate table, so
-	// it rides the same NodeWarnings channel as an unattributable one and
+	// it rides the same NodeWarnings channel as an unattributable one, through
-	// reaches every output path.
+	// every output path and through a failed load.
 	facts := puppetdb.New(opts.puppetdb, opts.timeout)
-	var dropped []string
+	var dropped []impact.NodeWarning
-	facts.Warn = func(msg string) { dropped = append(dropped, msg) }
+	facts.Warn = func(msg string, records int) {
-
+		dropped = append(dropped, impact.NodeWarning{Message: msg, Records: records})
-	nodes, nodeWarnings, err := impact.LoadNodes(ctx, facts)
-	if err != nil {
-		return err
 	}
 
-	analyzer := &impact.Analyzer{Index: idx, Nodes: nodes, NodeWarnings: append(dropped, nodeWarnings...)}
+	nodes, nodeWarnings, err := impact.LoadNodes(ctx, facts)
+	warnings := append(dropped, nodeWarnings...)
+	if err != nil {
+		return loadFailed(errOut, warnings, err)
+	}
+
+	analyzer := &impact.Analyzer{Index: idx, Nodes: nodes, NodeWarnings: warnings}
 	result := analyzer.Analyze(opts.base, branch, files, opts.limit)
 
 	switch {
@@ -154,9 +157,7 @@ func warn(w io.Writer, r *impact.Result) {
 	for _, line := range impact.UnresolvedLines(r.UnresolvedRefs) {
 		_, _ = fmt.Fprintf(w, "warning: %s did not resolve to a class name; the host list may be incomplete\n", line)
 	}
-	for _, line := range r.NodeWarnings {
+	warnNodes(w, r.NodeWarnings)
-		_, _ = fmt.Fprintf(w, "warning: %s; the estate table may be missing hosts\n", line)
-	}
 	for _, class := range r.OrphanClasses {
 		_, _ = fmt.Fprintf(w, "warning: class %s is declared by nothing and listed in no hiera_include; it reaches no hosts\n", class)
 	}
@@ -164,3 +165,20 @@ func warn(w io.Writer, r *impact.Result) {
 		_, _ = fmt.Fprintf(w, "warning: %d hosts affected, output capped at %d by --limit\n", r.Summary.Hosts, len(r.Hosts))
 	}
 }
+
+func warnNodes(w io.Writer, lines []string) {
+	for _, line := range lines {
+		_, _ = fmt.Fprintf(w, "warning: %s; the estate table may be missing hosts\n", line)
+	}
+}
+
+// loadFailed keeps the warnings that explain a failed load from dying with it:
+// no report is rendered once the run stops here, so they go to stderr in every
+// output mode and the tally rides on the error.
+func loadFailed(w io.Writer, warnings []impact.NodeWarning, err error) error {
+	warnNodes(w, impact.NodeWarningMessages(warnings))
+	if n := impact.DroppedRecords(warnings); n > 0 {
+		return fmt.Errorf("%w (%d fact record(s) dropped)", err, n)
+	}
+	return err
+}

cmd/catalog-impact/main_test.go

@@ -127,14 +127,17 @@ func TestRunReportsDroppedFactRecordsOnStderr(t *testing.T) {
 	if err := run(context.Background(), &out, &errOut, droppedFactOptions(t)); err != nil {
 		t.Fatalf("run: %v", err)
 	}
-	if strings.Contains(out.String(), "web1.example.net") {
+	if out.String() != "" {
 		t.Fatalf("web1 should have dropped out of the estate table, output = %q", out.String())
 	}
-	for _, want := range []string{"undecodable fact(s)", "the estate table may be missing hosts"} {
+	for _, want := range []string{"skipped 3 undecodable fact(s)", "the estate table may be missing hosts"} {
 		if !strings.Contains(errOut.String(), want) {
 			t.Errorf("stderr = %q, want it to mention %q", errOut.String(), want)
 		}
 	}
+	if got := strings.Count(errOut.String(), "\n"); got != 1 {
+		t.Errorf("stderr has %d warning line(s), want 1: %q", got, errOut.String())
+	}
 }
 
 func TestRunReportsDroppedFactRecordsInJSON(t *testing.T) {
@@ -149,17 +152,21 @@ func TestRunReportsDroppedFactRecordsInJSON(t *testing.T) {
 		NodeWarnings []string `json:"node_warnings"`
 		Summary      struct {
 			ResolutionIncomplete bool `json:"resolution_incomplete"`
+			DroppedFactRecords   int  `json:"dropped_fact_records"`
 		} `json:"summary"`
 	}
 	if err := json.Unmarshal(out.Bytes(), &doc); err != nil {
 		t.Fatalf("decoding --json output: %v", err)
 	}
-	if len(doc.NodeWarnings) == 0 {
+	if len(doc.NodeWarnings) != 1 {
-		t.Error("node_warnings is empty; the dropped records went unreported")
+		t.Errorf("node_warnings = %v, want the one batched warning", doc.NodeWarnings)
 	}
 	if !doc.Summary.ResolutionIncomplete {
 		t.Error("resolution_incomplete is false after records were dropped")
 	}
+	if doc.Summary.DroppedFactRecords != 3 {
+		t.Errorf("summary.dropped_fact_records = %d, want 3", doc.Summary.DroppedFactRecords)
+	}
 }
 
 func TestRunReportsDroppedFactRecordsInExplain(t *testing.T) {
@@ -169,9 +176,60 @@ func TestRunReportsDroppedFactRecordsInExplain(t *testing.T) {
 	if err := run(context.Background(), &out, io.Discard, opts); err != nil {
 		t.Fatalf("run: %v", err)
 	}
-	for _, want := range []string{"fact records dropped", "resolution incomplete", "undecodable fact(s)"} {
+	// web1 dropped three records in one warning, so the headline must count
+	// records, not messages.
+	for _, want := range []string{
+		"fact records dropped",
+		"3 fact record(s) dropped",
+		"skipped 3 undecodable fact(s)",
+	} {
 		if !strings.Contains(out.String(), want) {
 			t.Errorf("explain output = %q, want it to mention %q", out.String(), want)
 		}
 	}
 }
+
+// allUndecodableEstateBody strips the value from every record of every host,
+// so the estate table comes back empty and the load fails.
+const allUndecodableEstateBody = `[
+  {"certname":"web1.example.net","name":"enc_role"},
+  {"certname":"web1.example.net","name":"country"},
+  {"certname":"k8s1.example.net","name":"enc_role"},
+  {"certname":"k8s1.example.net","name":"country"}
+]`
+
+func TestRunReportsDroppedFactRecordsWhenTheLoadFails(t *testing.T) {
+	for _, tc := range []struct {
+		name    string
+		jsonOut bool
+		explain bool
+	}{
+		{name: "plain"},
+		{name: "json", jsonOut: true},
+		{name: "explain", explain: true},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			var out, errOut bytes.Buffer
+			opts := &options{
+				repo:     "../../internal/impact/testdata/repo",
+				base:     "origin/develop",
+				files:    []string{"modules/jellyfin/manifests/install.pp"},
+				puppetdb: fakePuppetDBBody(t, allUndecodableEstateBody),
+				jsonOut:  tc.jsonOut,
+				explain:  tc.explain,
+				timeout:  5 * time.Second,
+			}
+
+			err := run(context.Background(), &out, &errOut, opts)
+			if err == nil {
+				t.Fatal("an empty estate table must fail the run")
+			}
+			if !strings.Contains(err.Error(), "4 fact record(s) dropped") {
+				t.Errorf("error = %q, want it to account for the 4 dropped records", err)
+			}
+			if !strings.Contains(errOut.String(), "skipped 4 undecodable fact(s)") {
+				t.Errorf("stderr = %q, want the dropped records reported", errOut.String())
+			}
+		})
+	}
+}

internal/impact/impact.go

@@ -27,6 +27,9 @@ type Summary struct {
 	// statically, or a fact record could not be attributed to a host, so the
 	// answer is a best effort rather than exact.
 	ResolutionIncomplete bool `json:"resolution_incomplete"`
+	// DroppedFactRecords counts the fact records that never reached the estate
+	// table, which is not the number of warnings: one warning can cover many.
+	DroppedFactRecords int `json:"dropped_fact_records"`
 }
 
 // Result is the whole analysis, and the --json document.
@@ -73,7 +76,7 @@ type Analyzer struct {
 	Index *Index
 	Nodes []Node
 	// NodeWarnings carries whatever LoadNodes could not fold into Nodes.
-	NodeWarnings []string
+	NodeWarnings []NodeWarning
 }
 
 // Analyze maps changed files onto hosts. Files are processed in the order
@@ -91,7 +94,7 @@ func (a *Analyzer) Analyze(base, branch string, files []string, limit int) *Resu
 		Base:         base,
 		Branch:       branch,
 		ChangedFiles: uniqueSorted(files),
-		NodeWarnings: a.NodeWarnings,
+		NodeWarnings: NodeWarningMessages(a.NodeWarnings),
 		TotalHosts:   len(a.Nodes),
 	}
 
@@ -162,6 +165,7 @@ func (a *Analyzer) Analyze(base, branch string, files []string, limit int) *Resu
 		Files:                len(result.ChangedFiles),
 		AllHostsTriggered:    len(certnames) > 0 && len(certnames) == len(a.Nodes),
 		ResolutionIncomplete: len(result.UnresolvedRefs) > 0 || len(result.NodeWarnings) > 0,
+		DroppedFactRecords:   DroppedRecords(a.NodeWarnings),
 	}
 
 	if limit > 0 && len(certnames) > limit {

internal/impact/impact_test.go

@@ -275,9 +275,9 @@ func TestLoadNodesReportsRecordsWithNoCertname(t *testing.T) {
 	if len(nodes) != 1 {
 		t.Fatalf("nodes = %#v", nodes)
 	}
-	want := []string{
+	want := []NodeWarning{
-		`fact "country": 1 record(s) had no certname and were dropped`,
+		{Message: `fact "country": 1 record(s) had no certname and were dropped`, Records: 1},
-		`fact "enc_role": 2 record(s) had no certname and were dropped`,
+		{Message: `fact "enc_role": 2 record(s) had no certname and were dropped`, Records: 2},
 	}
 	if !reflect.DeepEqual(warnings, want) {
 		t.Errorf("warnings = %v, want %v", warnings, want)
@@ -285,12 +285,36 @@ func TestLoadNodesReportsRecordsWithNoCertname(t *testing.T) {
 
 	r := (&Analyzer{Index: testIndex(t), Nodes: nodes, NodeWarnings: warnings}).
 		Analyze("base", "branch", []string{"README.md"}, 0)
-	if !reflect.DeepEqual(r.NodeWarnings, want) {
+	if !reflect.DeepEqual(r.NodeWarnings, NodeWarningMessages(want)) {
-		t.Errorf("Result.NodeWarnings = %v, want %v", r.NodeWarnings, want)
+		t.Errorf("Result.NodeWarnings = %v, want %v", r.NodeWarnings, NodeWarningMessages(want))
 	}
 	if !r.Summary.ResolutionIncomplete {
 		t.Error("a dropped fact record left the summary looking exact")
 	}
+	// Three records in two messages: the summary counts records.
+	if r.Summary.DroppedFactRecords != 3 {
+		t.Errorf("Summary.DroppedFactRecords = %d, want 3", r.Summary.DroppedFactRecords)
+	}
+}
+
+func TestLoadNodesReturnsWarningsWithItsError(t *testing.T) {
+	// Every record is unattributable, so the table is empty and the load
+	// fails. The warnings are the only explanation and must survive the error.
+	src := &stubFacts{records: []puppetdb.FactRecord{
+		{Name: "enc_role", Value: "roles::base"},
+		{Name: "enc_role", Value: "roles::web"},
+	}}
+
+	nodes, warnings, err := LoadNodes(context.Background(), src)
+	if err == nil {
+		t.Fatal("expected an error for an estate table with no attributable records")
+	}
+	if len(nodes) != 0 {
+		t.Fatalf("nodes = %#v", nodes)
+	}
+	if got := DroppedRecords(warnings); got != 2 {
+		t.Errorf("warnings account for %d dropped record(s), want 2", got)
+	}
 }
 
 func TestAnalyzeReportsFilesThatReachNoHost(t *testing.T) {

internal/impact/node.go

@@ -56,11 +56,40 @@ type FactSource interface {
 	FactsByName(ctx context.Context, names []string) ([]puppetdb.FactRecord, error)
 }
 
+// NodeWarning is one report of fact records that never reached the estate
+// table, with the number of records it accounts for. One warning can stand for
+// many records, so the count travels with the message rather than being
+// inferred from it.
+type NodeWarning struct {
+	Message string
+	Records int
+}
+
+// NodeWarningMessages renders warnings one line each.
+func NodeWarningMessages(warnings []NodeWarning) []string {
+	out := make([]string, 0, len(warnings))
+	for _, w := range warnings {
+		out = append(out, w.Message)
+	}
+	return out
+}
+
+// DroppedRecords totals the records the warnings account for.
+func DroppedRecords(warnings []NodeWarning) int {
+	total := 0
+	for _, w := range warnings {
+		total += w.Records
+	}
+	return total
+}
+
 // LoadNodes fetches FactNames for every node and folds them into a sorted
 // table. Nodes without an enc_role are kept: they still match country, os and
 // virtual selectors. A record with no certname belongs to no host and cannot
 // be kept, so it is returned as a warning rather than dropped in silence.
-func LoadNodes(ctx context.Context, src FactSource) ([]Node, []string, error) {
+// Warnings are returned alongside a failure too: they are usually what
+// explains it.
+func LoadNodes(ctx context.Context, src FactSource) ([]Node, []NodeWarning, error) {
 	records, err := src.FactsByName(ctx, FactNames)
 	if err != nil {
 		return nil, nil, err
@@ -93,15 +122,18 @@ func LoadNodes(ctx context.Context, src FactSource) ([]Node, []string, error) {
 }
 
 // anonymousWarnings renders the dropped-record tally one line per fact.
-func anonymousWarnings(counts map[string]int) []string {
+func anonymousWarnings(counts map[string]int) []NodeWarning {
 	names := make([]string, 0, len(counts))
 	for name := range counts {
 		names = append(names, name)
 	}
 	sort.Strings(names)
-	out := make([]string, 0, len(names))
+	out := make([]NodeWarning, 0, len(names))
 	for _, name := range names {
-		out = append(out, fmt.Sprintf("fact %q: %d record(s) had no certname and were dropped", name, counts[name]))
+		out = append(out, NodeWarning{
+			Message: fmt.Sprintf("fact %q: %d record(s) had no certname and were dropped", name, counts[name]),
+			Records: counts[name],
+		})
 	}
 	return out
 }

internal/impact/render.go

@@ -64,7 +64,7 @@ func RenderExplain(w io.Writer, r *Result) error {
 	}
 	if r.Summary.ResolutionIncomplete {
 		if err := p("resolution incomplete: %d class reference(s) unresolved, %d fact record(s) dropped",
-			len(r.UnresolvedRefs), len(r.NodeWarnings)); err != nil {
+			len(r.UnresolvedRefs), r.Summary.DroppedFactRecords); err != nil {
 			return err
 		}
 	}

internal/puppetdb/puppetdb.go

@@ -25,10 +25,13 @@ type Client struct {
 	BaseURL string
 	HTTP    *http.Client
 
-	// Warn receives a message for every fact record that could not be decoded
+	// Warn receives one message per batch of dropped fact records, with the
-	// and was therefore dropped. A nil Warn reports to stderr rather than
+	// number of records that batch accounts for — batches differ in size, so
-	// discarding: a dropped record must never be silent.
+	// counting messages is not counting records. A nil Warn reports to stderr
-	Warn func(string)
+	// rather than discarding: a dropped record must never be silent. It is
+	// called on the goroutine that made the request, so a Client shared across
+	// goroutines needs a Warn that is safe to call concurrently.
+	Warn func(msg string, records int)
 }
 
 // New returns a client for baseURL.
@@ -115,7 +118,7 @@ func (c *Client) FactsByName(ctx context.Context, names []string) ([]FactRecord,
 		out = append(out, FactRecord{Certname: r.Certname, Name: r.Name, Value: v})
 	}
 	if len(skipped) > 0 {
-		c.warn("skipped %d undecodable fact(s) in the estate fact table: %s",
+		c.warn(len(skipped), "skipped %d undecodable fact(s) in the estate fact table: %s",
 			len(skipped), strings.Join(skipped, ", "))
 	}
 	return out, nil
@@ -247,18 +250,18 @@ func (c *Client) fold(certname string, records []factRecord) (map[string]any, er
 		return nil, fmt.Errorf("no usable facts for %s: all %d records were undecodable", certname, len(records))
 	}
 	if len(skipped) > 0 {
-		c.warn("skipped %d undecodable fact(s) for %s: %s",
+		c.warn(len(skipped), "skipped %d undecodable fact(s) for %s: %s",
 			len(skipped), certname, strings.Join(skipped, ", "))
 	}
 	return values, nil
 }
 
-// warn reports a dropped record. An unwired hook falls back to stderr so that
+// warn reports records dropped. An unwired hook falls back to stderr so that
 // forgetting to set one loses visibility, never the record itself.
-func (c *Client) warn(format string, args ...any) {
+func (c *Client) warn(records int, format string, args ...any) {
 	msg := fmt.Sprintf(format, args...)
 	if c.Warn != nil {
-		c.Warn(msg)
+		c.Warn(msg, records)
 		return
 	}
 	fmt.Fprintln(os.Stderr, "warning:", msg)

internal/puppetdb/puppetdb_test.go

@@ -291,8 +291,12 @@ func TestFactsSkipsUndecodableRecords(t *testing.T) {
 	]`)
 
 	var warnings []string
+	var counted int
 	c := New(srv.URL, 5*time.Second)
-	c.Warn = func(msg string) { warnings = append(warnings, msg) }
+	c.Warn = func(msg string, records int) {
+		warnings = append(warnings, msg)
+		counted += records
+	}
 
 	facts, err := c.Facts(context.Background(), "host.example.net")
 	if err != nil {
@@ -305,6 +309,9 @@ func TestFactsSkipsUndecodableRecords(t *testing.T) {
 	if len(warnings) != 1 || !strings.Contains(warnings[0], "broken") {
 		t.Errorf("the skipped fact must be reported, got %v", warnings)
 	}
+	if counted != 1 {
+		t.Errorf("Warn reported %d dropped record(s), want 1", counted)
+	}
 }
 
 func TestFactsWithNothingDecodableIsAnError(t *testing.T) {
@@ -383,14 +390,20 @@ func TestFactsByNameSkipsUndecodableRecords(t *testing.T) {
 		w.Header().Set("Content-Type", "application/json")
 		_, _ = w.Write([]byte("\ufeff" + `[
 		  {"certname":"a.example.net","name":"country","value":"au"},
-		  {"certname":"a.example.net","name":"broken"}
+		  {"certname":"a.example.net","name":"broken"},
+		  {"certname":"b.example.net","name":"broken"},
+		  {"certname":"c.example.net","name":"broken"}
 		]`))
 	}))
 	defer srv.Close()
 
 	var warnings []string
+	var counted int
 	c := New(srv.URL, 5*time.Second)
-	c.Warn = func(msg string) { warnings = append(warnings, msg) }
+	c.Warn = func(msg string, records int) {
+		warnings = append(warnings, msg)
+		counted += records
+	}
 
 	records, err := c.FactsByName(context.Background(), []string{"country", "broken"})
 	if err != nil {
@@ -399,9 +412,14 @@ func TestFactsByNameSkipsUndecodableRecords(t *testing.T) {
 	if len(records) != 1 || records[0].Name != "country" {
 		t.Fatalf("records = %#v", records)
 	}
+	// One query batches its drops into one message, so the count has to come
+	// from the hook and not from the number of messages.
 	if len(warnings) != 1 || !strings.Contains(warnings[0], "broken") {
 		t.Errorf("the skipped fact must be reported, got %v", warnings)
 	}
+	if counted != 3 {
+		t.Errorf("Warn reported %d dropped record(s), want 3", counted)
+	}
 }
 
 func TestFactsByNameWithoutWarnHookReportsOnStderr(t *testing.T) {