Merge pull request 'Report fact records dropped from the estate table' (#3) from benvin/report-dropped-fact-records into main

Reviewed-on: #3

README.md

@@ -242,10 +242,13 @@ A missed host means an unvalidated catalog, so every ambiguity widens:
   `--explain`, in `unresolved_references` in `--json`, on stderr in plain mode,
   and set `summary.resolution_incomplete`.
 
-A fact record that arrives with no certname belongs to no host and cannot be
-folded into the estate table, so it is counted per fact and reported in
-`node_warnings`, in `--explain` and on stderr, and sets
-`summary.resolution_incomplete` too.
+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, counted in
+`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

@@ -67,7 +67,7 @@ func main() {
 		SilenceUsage:  true,
 		SilenceErrors: true,
 		RunE: func(cmd *cobra.Command, args []string) error {
-			return run(cmd.Context(), cmd.OutOrStdout(), opts)
+			return run(cmd.Context(), cmd.OutOrStdout(), cmd.ErrOrStderr(), opts)
 		},
 	}
 
@@ -89,7 +89,7 @@ func main() {
 	os.Exit(exitOK)
 }
 
-func run(ctx context.Context, out io.Writer, opts *options) error {
+func run(ctx context.Context, out, errOut io.Writer, opts *options) error {
 	if opts.jsonOut && opts.explain {
 		return fmt.Errorf("--json and --explain are mutually exclusive")
 	}
@@ -119,12 +119,22 @@ func run(ctx context.Context, out io.Writer, opts *options) error {
 		return fmt.Errorf("indexing %s: %w", opts.repo, err)
 	}
 
-	nodes, nodeWarnings, err := impact.LoadNodes(ctx, puppetdb.New(opts.puppetdb, opts.timeout))
-	if err != nil {
-		return err
+	// 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, through
+	// every output path and through a failed load.
+	facts := puppetdb.New(opts.puppetdb, opts.timeout)
+	var dropped []impact.NodeWarning
+	facts.Warn = func(msg string, records int) {
+		dropped = append(dropped, impact.NodeWarning{Message: msg, Records: records})
 	}
 
-	analyzer := &impact.Analyzer{Index: idx, Nodes: nodes, NodeWarnings: 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 {
@@ -134,26 +144,41 @@ func run(ctx context.Context, out io.Writer, opts *options) error {
 		return impact.RenderExplain(out, result)
 	}
 
-	warn(result)
+	warn(errOut, result)
 	return impact.RenderPlain(out, result)
 }
 
 // warn surfaces the widenings and dead ends on stderr so a piped host list
 // stays clean.
-func warn(r *impact.Result) {
+func warn(w io.Writer, r *impact.Result) {
 	for _, file := range r.UnmappedFiles {
-		fmt.Fprintf(os.Stderr, "warning: %s matches no mapping rule; every host is included\n", file)
+		_, _ = fmt.Fprintf(w, "warning: %s matches no mapping rule; every host is included\n", file)
 	}
 	for _, line := range impact.UnresolvedLines(r.UnresolvedRefs) {
-		fmt.Fprintf(os.Stderr, "warning: %s did not resolve to a class name; the host list may be incomplete\n", line)
-	}
-	for _, line := range r.NodeWarnings {
-		fmt.Fprintf(os.Stderr, "warning: %s; the estate table may be missing hosts\n", line)
+		_, _ = fmt.Fprintf(w, "warning: %s did not resolve to a class name; the host list may be incomplete\n", line)
 	}
+	warnNodes(w, r.NodeWarnings)
 	for _, class := range r.OrphanClasses {
-		fmt.Fprintf(os.Stderr, "warning: class %s is declared by nothing and listed in no hiera_include; it reaches no hosts\n", class)
+		_, _ = fmt.Fprintf(w, "warning: class %s is declared by nothing and listed in no hiera_include; it reaches no hosts\n", class)
 	}
 	if r.Truncated {
-		fmt.Fprintf(os.Stderr, "warning: %d hosts affected, output capped at %d by --limit\n", r.Summary.Hosts, len(r.Hosts))
+		_, _ = 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

@@ -3,6 +3,8 @@ package main
 import (
 	"bytes"
 	"context"
+	"encoding/json"
+	"io"
 	"net/http"
 	"net/http/httptest"
 	"strings"
@@ -48,7 +50,7 @@ func TestRunWithExplicitFileList(t *testing.T) {
 		puppetdb: fakePuppetDB(t),
 		timeout:  5 * time.Second,
 	}
-	if err := run(context.Background(), &out, opts); err != nil {
+	if err := run(context.Background(), &out, io.Discard, opts); err != nil {
 		t.Fatalf("run: %v", err)
 	}
 	if got, want := out.String(), "web1.example.net\n"; got != want {
@@ -66,7 +68,7 @@ func TestRunExplain(t *testing.T) {
 		explain:  true,
 		timeout:  5 * time.Second,
 	}
-	if err := run(context.Background(), &out, opts); err != nil {
+	if err := run(context.Background(), &out, io.Discard, opts); err != nil {
 		t.Fatalf("run: %v", err)
 	}
 	if !strings.Contains(out.String(), "the whole estate is affected") {
@@ -76,14 +78,158 @@ func TestRunExplain(t *testing.T) {
 
 func TestRunRejectsConflictingOutputFlags(t *testing.T) {
 	opts := &options{jsonOut: true, explain: true}
-	if err := run(context.Background(), &bytes.Buffer{}, opts); err == nil {
+	if err := run(context.Background(), &bytes.Buffer{}, io.Discard, opts); err == nil {
 		t.Fatal("expected an error for --json with --explain")
 	}
 }
 
 func TestRunRejectsNegativeLimit(t *testing.T) {
 	opts := &options{limit: -1}
-	if err := run(context.Background(), &bytes.Buffer{}, opts); err == nil {
+	if err := run(context.Background(), &bytes.Buffer{}, io.Discard, opts); err == nil {
 		t.Fatal("expected an error for a negative --limit")
 	}
 }
+
+// undecodableEstateBody strips the value from every one of web1's records, so
+// each decodes to nothing and the host leaves the estate table with it.
+const undecodableEstateBody = `[
+  {"certname":"web1.example.net","name":"enc_role"},
+  {"certname":"web1.example.net","name":"country"},
+  {"certname":"web1.example.net","name":"os"},
+  {"certname":"k8s1.example.net","name":"enc_role","value":"roles::infra::k8s::node"},
+  {"certname":"k8s1.example.net","name":"country","value":"au"},
+  {"certname":"k8s1.example.net","name":"os","value":{"name":"AlmaLinux","release":{"major":"9","minor":"8"}}}
+]`
+
+func fakePuppetDBBody(t *testing.T, body string) string {
+	t.Helper()
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = w.Write([]byte(body))
+	}))
+	t.Cleanup(srv.Close)
+	return srv.URL
+}
+
+func droppedFactOptions(t *testing.T) *options {
+	t.Helper()
+	return &options{
+		repo:     "../../internal/impact/testdata/repo",
+		base:     "origin/develop",
+		files:    []string{"modules/jellyfin/manifests/install.pp"},
+		puppetdb: fakePuppetDBBody(t, undecodableEstateBody),
+		timeout:  5 * time.Second,
+	}
+}
+
+func TestRunReportsDroppedFactRecordsOnStderr(t *testing.T) {
+	var out, errOut bytes.Buffer
+	if err := run(context.Background(), &out, &errOut, droppedFactOptions(t)); err != nil {
+		t.Fatalf("run: %v", err)
+	}
+	if out.String() != "" {
+		t.Fatalf("web1 should have dropped out of the estate table, output = %q", out.String())
+	}
+	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) {
+	var out bytes.Buffer
+	opts := droppedFactOptions(t)
+	opts.jsonOut = true
+	if err := run(context.Background(), &out, io.Discard, opts); err != nil {
+		t.Fatalf("run: %v", err)
+	}
+
+	var doc struct {
+		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) != 1 {
+		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) {
+	var out bytes.Buffer
+	opts := droppedFactOptions(t)
+	opts.explain = true
+	if err := run(context.Background(), &out, io.Discard, opts); err != nil {
+		t.Fatalf("run: %v", err)
+	}
+	// 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{
-		`fact "country": 1 record(s) had no certname and were dropped`,
-		`fact "enc_role": 2 record(s) had no certname and were dropped`,
+	want := []NodeWarning{
+		{Message: `fact "country": 1 record(s) had no certname and were dropped`, Records: 1},
+		{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) {
-		t.Errorf("Result.NodeWarnings = %v, want %v", r.NodeWarnings, want)
+	if !reflect.DeepEqual(r.NodeWarnings, NodeWarningMessages(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

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

internal/puppetdb/puppetdb.go

@@ -12,6 +12,7 @@ import (
 	"io"
 	"net/http"
 	"net/url"
+	"os"
 	"strings"
 	"time"
 )
@@ -24,9 +25,13 @@ type Client struct {
 	BaseURL string
 	HTTP    *http.Client
 
-	// Warn, when set, receives a message for every fact record that could not
-	// be decoded and was therefore dropped.
-	Warn func(string)
+	// Warn receives one message per batch of dropped fact records, with the
+	// number of records that batch accounts for — batches differ in size, so
+	// counting messages is not counting records. A nil Warn reports to stderr
+	// 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.
@@ -112,9 +117,9 @@ 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 != nil {
-		c.Warn(fmt.Sprintf("skipped %d undecodable fact(s) in the estate fact table: %s",
-			len(skipped), strings.Join(skipped, ", ")))
+	if len(skipped) > 0 {
+		c.warn(len(skipped), "skipped %d undecodable fact(s) in the estate fact table: %s",
+			len(skipped), strings.Join(skipped, ", "))
 	}
 	return out, nil
 }
@@ -244,13 +249,24 @@ func (c *Client) fold(certname string, records []factRecord) (map[string]any, er
 	if len(values) == 0 {
 		return nil, fmt.Errorf("no usable facts for %s: all %d records were undecodable", certname, len(records))
 	}
-	if len(skipped) > 0 && c.Warn != nil {
-		c.Warn(fmt.Sprintf("skipped %d undecodable fact(s) for %s: %s",
-			len(skipped), certname, strings.Join(skipped, ", ")))
+	if len(skipped) > 0 {
+		c.warn(len(skipped), "skipped %d undecodable fact(s) for %s: %s",
+			len(skipped), certname, strings.Join(skipped, ", "))
 	}
 	return values, nil
 }
 
+// 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(records int, format string, args ...any) {
+	msg := fmt.Sprintf(format, args...)
+	if c.Warn != nil {
+		c.Warn(msg, records)
+		return
+	}
+	fmt.Fprintln(os.Stderr, "warning:", msg)
+}
+
 func snippet(body []byte) string {
 	s := strings.TrimSpace(string(body))
 	if len(s) > 200 {

internal/puppetdb/puppetdb_test.go

@@ -4,9 +4,12 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
+	"io"
 	"net/http"
 	"net/http/httptest"
+	"os"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 )
@@ -288,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 {
@@ -302,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) {
@@ -380,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 {
@@ -396,7 +412,55 @@ 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) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = w.Write([]byte(`[{"certname":"a.example.net","name":"broken"}]`))
+	}))
+	defer srv.Close()
+
+	stderr := captureStderr(t)
+	if _, err := New(srv.URL, 5*time.Second).FactsByName(context.Background(), []string{"broken"}); err != nil {
+		t.Fatalf("FactsByName: %v", err)
+	}
+	if got := stderr(); !strings.Contains(got, "broken") {
+		t.Errorf("an unwired Warn must still report the drop, stderr = %q", got)
+	}
+}
+
+// captureStderr redirects os.Stderr for the duration of the test and returns a
+// func that restores it and yields what was written.
+func captureStderr(t *testing.T) func() string {
+	t.Helper()
+	r, w, err := os.Pipe()
+	if err != nil {
+		t.Fatalf("pipe: %v", err)
+	}
+	saved := os.Stderr
+	os.Stderr = w
+
+	var once sync.Once
+	var out string
+	read := func() string {
+		once.Do(func() {
+			os.Stderr = saved
+			_ = w.Close()
+			b, _ := io.ReadAll(r)
+			_ = r.Close()
+			out = string(b)
+		})
+		return out
+	}
+	t.Cleanup(func() { read() })
+	return read
 }