Report fact records dropped from the estate table #3

2026-09-12T20:10:07+10:00

unkin-agent commented

2026-09-12 20:10:07 +10:00

catalog-impact exists to over-include: a host missing from the estate table is a catalog nobody validates. Records dropped by PuppetDB were reported only when the load succeeded, and --explain counted warning messages rather than records, so one batch of N drops always read as "1".

report the accumulated warnings on stderr when the estate load fails, in every output mode, and fold the dropped-record tally into that error
carry a record count on the puppetdb Warn hook; count records in --explain and in summary.dropped_fact_records
default an unset Warn to stderr so forgetting the hook costs visibility, not the record
document that Warn must be safe to call concurrently when a Client is shared
assert the numbers, not just the text, in the dropped-record tests

`catalog-impact` exists to over-include: a host missing from the estate table is a catalog nobody validates. Records dropped by PuppetDB were reported only when the load succeeded, and `--explain` counted warning messages rather than records, so one batch of N drops always read as "1". - report the accumulated warnings on stderr when the estate load fails, in every output mode, and fold the dropped-record tally into that error - carry a record count on the puppetdb `Warn` hook; count records in `--explain` and in `summary.dropped_fact_records` - default an unset `Warn` to stderr so forgetting the hook costs visibility, not the record - document that `Warn` must be safe to call concurrently when a `Client` is shared - assert the numbers, not just the text, in the dropped-record tests

unkin-agent added 1 commit 2026-09-12 20:10:08 +10:00

report fact records dropped from the estate table

ci/woodpecker/pr/build Pipeline was successful

Details

ci/woodpecker/pr/test Pipeline failed

Details

ci/woodpecker/pr/pre-commit Pipeline was successful

Details

86e4958bcb

catalog-impact built its PuppetDB client without a Warn hook, so a host whose
fact records all failed to decode vanished from the estate table with no
warning on any output path.

- wire catalog-impact's Warn hook into NodeWarnings
- default an unset Warn to stderr so a dropped record is never silent
- thread stderr through run() instead of writing os.Stderr directly
- count dropped fact records in the explain resolution-incomplete line

unkin-agent added 1 commit 2026-09-12 20:12:15 +10:00

check the stderr writes errcheck now sees

ci/woodpecker/pr/pre-commit Pipeline was successful

Details

ci/woodpecker/pr/build Pipeline was successful

Details

ci/woodpecker/pr/test Pipeline was successful

Details

88bbc2f441

unkin-agent commented

2026-09-12 20:21:14 +10:00

Verdict: issues found — one blocking correctness bug in the new reporting path itself, plus a wrong count. Not atomicity/scope problems; tests are real (all 4 fail on reverted logic); go test -race ./..., make lint, make pre-commit all green.

Blocking

The new dropped-record reporting is itself lost on LoadNodes's error path. cmd/catalog-impact/main.go:
```
nodes, nodeWarnings, err := impact.LoadNodes(ctx, facts)
if err != nil {
    return err
}
```
facts.Warn populates dropped via closure, but dropped is never consulted here. Repro: point --puppetdb at a server that returns fact records for every host with no value field (all undecodable). FactsByName calls c.warn("skipped 2 undecodable fact(s)...") (captured into dropped) but returns an empty record slice; LoadNodes then has zero nodes and returns error: puppetdb returned no nodes. run() returns that error immediately — stdout, stderr, --json and --explain all say nothing but "puppetdb returned no nodes"; the actual reason never surfaces anywhere. Verified by running it. This is the exact silent-under-include class this PR exists to close, and it directly contradicts the new comment above it ("rides the same NodeWarnings channel ... reaches every output path") — it reaches none of them on this path. Same root cause as the internal/impact/node.go gap the PR body flags as unfixed (LoadNodes discards nodeWarnings on its own error return) — that gap should have been in scope here, since the fix just built now inherits it and loses its own new warnings too, not just the pre-existing anonymous-record ones.
The new explain-mode count is wrong. internal/impact/render.go prints "%d fact record(s) dropped" from len(r.NodeWarnings), but FactsByName batches every skipped record in one query into a single warning string ("skipped N undecodable fact(s)..."), so N dropped records always show as "1". Repro with the PR's own fixture (web1 has 3 undecodable records): explain output reads resolution incomplete: 0 class reference(s) unresolved, 1 fact record(s) dropped, immediately followed by skipped 3 undecodable fact(s) in the estate fact table: enc_role, country, os. The summary undercounts the real drop count; none of the new tests catch it since they only assert substrings, never the numbers.

Not blocking (would have done differently)

Design choice (default-to-stderr vs constructor arg): agree with the author — Go can't express "non-nil required," so a required constructor arg can still be nil and the fallback has to live somewhere regardless. No objection there. Minor: Warn's doc comment doesn't say the callback must be goroutine-safe; nothing calls it concurrently today, but a closure like dropped = append(...) would race if a future caller shared one *Client across goroutines (e.g. concurrent Facts() calls). Worth a one-line doc note, not a bug now.

Everything else — atomicity, PR body format (6 lines/733 chars, why + bullets, no narrative) — is clean.

Verdict: issues found — one blocking correctness bug in the new reporting path itself, plus a wrong count. Not atomicity/scope problems; tests are real (all 4 fail on reverted logic); `go test -race ./...`, `make lint`, `make pre-commit` all green. **Blocking** 1. The new dropped-record reporting is itself lost on `LoadNodes`'s error path. `cmd/catalog-impact/main.go`: ```go nodes, nodeWarnings, err := impact.LoadNodes(ctx, facts) if err != nil { return err } ``` `facts.Warn` populates `dropped` via closure, but `dropped` is never consulted here. Repro: point `--puppetdb` at a server that returns fact records for every host with no `value` field (all undecodable). `FactsByName` calls `c.warn("skipped 2 undecodable fact(s)...")` (captured into `dropped`) but returns an empty record slice; `LoadNodes` then has zero nodes and returns `error: puppetdb returned no nodes`. `run()` returns that error immediately — stdout, stderr, `--json` and `--explain` all say nothing but "puppetdb returned no nodes"; the actual reason never surfaces anywhere. Verified by running it. This is the exact silent-under-include class this PR exists to close, and it directly contradicts the new comment above it ("rides the same NodeWarnings channel ... reaches every output path") — it reaches none of them on this path. Same root cause as the `internal/impact/node.go` gap the PR body flags as unfixed (LoadNodes discards `nodeWarnings` on its own error return) — that gap should have been in scope here, since the fix just built now inherits it and loses its own new warnings too, not just the pre-existing anonymous-record ones. 2. The new explain-mode count is wrong. `internal/impact/render.go` prints `"%d fact record(s) dropped"` from `len(r.NodeWarnings)`, but `FactsByName` batches every skipped record in one query into a *single* warning string ("skipped N undecodable fact(s)..."), so N dropped records always show as "1". Repro with the PR's own fixture (web1 has 3 undecodable records): explain output reads `resolution incomplete: 0 class reference(s) unresolved, 1 fact record(s) dropped`, immediately followed by `skipped 3 undecodable fact(s) in the estate fact table: enc_role, country, os`. The summary undercounts the real drop count; none of the new tests catch it since they only assert substrings, never the numbers. **Not blocking (would have done differently)** - Design choice (default-to-stderr vs constructor arg): agree with the author — Go can't express "non-nil required," so a required constructor arg can still be nil and the fallback has to live somewhere regardless. No objection there. Minor: `Warn`'s doc comment doesn't say the callback must be goroutine-safe; nothing calls it concurrently today, but a closure like `dropped = append(...)` would race if a future caller shared one `*Client` across goroutines (e.g. concurrent `Facts()` calls). Worth a one-line doc note, not a bug now. Everything else — atomicity, PR body format (6 lines/733 chars, why + bullets, no narrative) — is clean.

unkin-agent added 1 commit 2026-09-12 20:27:57 +10:00

carry dropped fact records through the failure path and count them

ci/woodpecker/pr/build Pipeline was successful

Details

ci/woodpecker/pr/pre-commit Pipeline was successful

Details

ci/woodpecker/pr/test Pipeline was successful

Details

04ec9e6d41

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.

benvin merged commit b81b49d006 into main

2026-09-12 20:33:59 +10:00

benvin deleted branch benvin/report-dropped-fact-records

2026-09-12 20:33:59 +10:00

unkin-agent commented

2026-09-12 20:36:24 +10:00

Reviewed 04ec9e6 in isolation (worktree checkout verified at that exact SHA). Verdict: clean, no blocking bugs found.

What I checked:

Traced every exit from LoadNodes/FactsByName: HTTP error, malformed JSON body, context cancellation, and a genuinely empty query result all fail loudly with no dropped-record claim (correct, since nothing was actually dropped in those cases) — no silent-loss path found.
The load-failure path: reverted README.md, both main.gos, impact.go, node.go, render.go, puppetdb.go back to the pre-fix commit while keeping the new tests → TestRunReportsDroppedFactRecordsWhenTheLoadFails fails in all three modes (plain/json/explain) exactly as claimed, plus two packages fail to compile. Restored the fix, go test -race ./..., make lint, make pre-commit all pass.
Count correctness: loadFailed/Summary.DroppedFactRecords sum NodeWarning.Records, not message count. Added a throwaway test mixing one undecodable-value drop with one no-certname drop in the same run — merged correctly to dropped_fact_records: 2, no double count, no lost source.
Signature change: Warn(msg, records) updated coherently at both call sites. catalog-diff discarding the count is fine — its messages already embed the count inline for a single host, no aggregate is computed there.
Schema: dropped_fact_records is additive; node_warnings stays []string via NodeWarningMessages.
PR body meets convention (7 lines, 794 chars, why + present-tense bullets, no history/person/AI references).

Non-blocking observations:

No test in this commit exercises the hook-drop and no-certname-drop sources merging in the same run (I verified it by hand, it's correct) — worth adding for regression safety.
On a failed load, --json callers get nothing on stdout and must parse the stderr/error text for the tally rather than a structured field. Consistent with how every other failure in this tool behaves, but worth confirming that's acceptable for scripted consumers before this pattern gets copied elsewhere.

Reviewed 04ec9e6 in isolation (worktree checkout verified at that exact SHA). Verdict: clean, no blocking bugs found. What I checked: - Traced every exit from `LoadNodes`/`FactsByName`: HTTP error, malformed JSON body, context cancellation, and a genuinely empty query result all fail loudly with no dropped-record claim (correct, since nothing was actually dropped in those cases) — no silent-loss path found. - The load-failure path: reverted `README.md`, both `main.go`s, `impact.go`, `node.go`, `render.go`, `puppetdb.go` back to the pre-fix commit while keeping the new tests → `TestRunReportsDroppedFactRecordsWhenTheLoadFails` fails in all three modes (plain/json/explain) exactly as claimed, plus two packages fail to compile. Restored the fix, `go test -race ./...`, `make lint`, `make pre-commit` all pass. - Count correctness: `loadFailed`/`Summary.DroppedFactRecords` sum `NodeWarning.Records`, not message count. Added a throwaway test mixing one undecodable-value drop with one no-certname drop in the same run — merged correctly to `dropped_fact_records: 2`, no double count, no lost source. - Signature change: `Warn(msg, records)` updated coherently at both call sites. `catalog-diff` discarding the count is fine — its messages already embed the count inline for a single host, no aggregate is computed there. - Schema: `dropped_fact_records` is additive; `node_warnings` stays `[]string` via `NodeWarningMessages`. - PR body meets convention (7 lines, 794 chars, why + present-tense bullets, no history/person/AI references). Non-blocking observations: - No test in this commit exercises the hook-drop and no-certname-drop sources merging in the same run (I verified it by hand, it's correct) — worth adding for regression safety. - On a failed load, `--json` callers get nothing on stdout and must parse the stderr/error text for the tally rather than a structured field. Consistent with how every other failure in this tool behaves, but worth confirming that's acceptable for scripted consumers before this pattern gets copied elsewhere.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: unkin/puppet-catalog-tools#3