Serve fact queries live and drop the cache headers #26

2026-09-13T13:35:10+10:00

unkin-agent commented

2026-09-13 13:35:10 +10:00

pdbmux has to be indistinguishable from PuppetDB, but the v0.3.0 response cache left fact answers up to 30s stale and added X-Cache and Age headers real PuppetDB never emits.

serve /facts, /facts/<name>[/<value>] and /fact-names live on every request
keep the in-memory cache on merged /nodes only
drop X-Cache and Age from every response; /healthz still reports cache state
answer successful queries with PuppetDB's application/json;charset=utf-8

pdbmux has to be indistinguishable from PuppetDB, but the v0.3.0 response cache left fact answers up to 30s stale and added `X-Cache` and `Age` headers real PuppetDB never emits. - serve `/facts`, `/facts/<name>[/<value>]` and `/fact-names` live on every request - keep the in-memory cache on merged `/nodes` only - drop `X-Cache` and `Age` from every response; `/healthz` still reports cache state - answer successful queries with PuppetDB's `application/json;charset=utf-8`

unkin-agent added 1 commit 2026-09-13 13:35:10 +10:00

Serve fact queries live and drop the cache headers

ci/woodpecker/pr/build Pipeline was successful

Details

ci/woodpecker/pr/test Pipeline was successful

Details

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

Details

24f6d73d5c

Fact answers must be as current as a backend's own, and X-Cache/Age are
headers PuppetDB never sends.

- serve /facts, /facts/<name>[/<value>] and /fact-names live on every request
- keep the in-memory cache on merged /nodes only
- drop X-Cache and Age everywhere; /healthz still reports cache state
- answer successful queries with PuppetDB's application/json;charset=utf-8

unkin-agent commented

2026-09-13 13:52:25 +10:00

Review findings.

Blocking / needs discussion

The load justification for dropping the /facts cache is factually wrong. puppetboard's /facts overview page (puppetboard/views/facts.py, routes /facts and /<env>/facts) calls puppetdb.facts(query=query) where query is AndOperator() filtered only by environment — or None entirely when "All Environments" is selected, a normal menu choice. That's a bare, non-name/certname-filtered fetch of every fact for every node (in-environment, or estate-wide with *). This is exactly the page pdbmux's own pre-PR README named as one of the two hardest-hitting paths the fact cache existed to absorb, not an edge case.
Removing /facts from cacheFor also silently drops single-flight coalescing for it: serveFiltered's !enabled branch calls build() directly, bypassing s.flights.Do. Combined with (1), two concurrent loads of the facts-overview page now each trigger a full two-backend, whole-estate fact fan-out instead of sharing one (or hitting a ≤30s cache). Worth an explicit call whether the backends can absorb that, since the PR body doesn't mention coalescing loss at all.

Verified independently: git grep on voxpupuli/pypuppetdb and voxpupuli/puppetboard shows neither reads X-Cache/Age, and neither validates response Content-Type (pypuppetdb parses via requests' .json()), so those two header/content-type changes are safe client-side.

Non-blocking

Content-type comment (server.go jsonContentType) cites http.clj:80, but that line in OpenVoxProject/openvoxdb literally sets "application/json; charset=utf-8" (with a space); the deployed backends actually emit application/json;charset=utf-8 (no space, confirmed live against both puppetdbapi.service.consul:8080 and puppetdb.k8s.syd1.au.unkin.net), which is what the PR uses. Value is correct, citation doesn't explain the discrepancy.
cacheFor/cache-key logic, error-path content-type, and every other header are otherwise correctly untouched and complete: no fact-bearing route stayed cached, and /nodes is the only route the cache still covers. Confirmed against a live /nodes record that it carries only report/run-lifecycle fields (certname, deactivated/expired, catalog/facts/report timestamps, environments, latest_report_*) and no fact key/value data, so a ≤30s-stale /nodes read cannot resurface a stale fact — the "report caching" reading is sound. That staleness window on latest_report_status/deactivated/expired already existed pre-PR (nodes was already cached), so it's not a new regression.
Reverting the cacheFor switch + header/content-type code on the PR branch reproduces failures in 6 tests (TestHandler_CachedResponsesCarryNoCacheHeaders, TestHandler_FactPathsAreNeverCached, TestHandler_FactResponsesCarryNoCacheHeaders, TestHandler_SuccessContentTypeMatchesPuppetDB, TestHandler_FactsBySourceNameValuesFilterOneFetch, TestHandler_FactRoutesAreNotCached) — close to the claimed five and confirms they aren't vacuous. The deleted fact+cache-interaction tests (source attribution, per-key gating, suppression-survives-hit, stale drilldown filtering) have live equivalents already in the untouched source_test.go/factroutes_test.go, so coverage moved rather than disappeared.
go test -race ./..., golangci-lint run ./..., and pre-commit run --all-files all pass clean on the PR branch; e2e files still compile under -tags e2e.
PR body meets conventions (why + present-tense how, 6 lines, 476 chars).

Review findings. **Blocking / needs discussion** 1. The load justification for dropping the `/facts` cache is factually wrong. puppetboard's `/facts` overview page (`puppetboard/views/facts.py`, routes `/facts` and `/<env>/facts`) calls `puppetdb.facts(query=query)` where `query` is `AndOperator()` filtered only by `environment` — or `None` entirely when "All Environments" is selected, a normal menu choice. That's a bare, non-name/certname-filtered fetch of every fact for every node (in-environment, or estate-wide with `*`). This is exactly the page pdbmux's own pre-PR README named as one of the two hardest-hitting paths the fact cache existed to absorb, not an edge case. 2. Removing `/facts` from `cacheFor` also silently drops single-flight coalescing for it: `serveFiltered`'s `!enabled` branch calls `build()` directly, bypassing `s.flights.Do`. Combined with (1), two concurrent loads of the facts-overview page now each trigger a full two-backend, whole-estate fact fan-out instead of sharing one (or hitting a ≤30s cache). Worth an explicit call whether the backends can absorb that, since the PR body doesn't mention coalescing loss at all. Verified independently: `git grep` on voxpupuli/pypuppetdb and voxpupuli/puppetboard shows neither reads `X-Cache`/`Age`, and neither validates response `Content-Type` (pypuppetdb parses via `requests`' `.json()`), so those two header/content-type changes are safe client-side. **Non-blocking** - Content-type comment (`server.go` `jsonContentType`) cites `http.clj:80`, but that line in OpenVoxProject/openvoxdb literally sets `"application/json; charset=utf-8"` (with a space); the deployed backends actually emit `application/json;charset=utf-8` (no space, confirmed live against both `puppetdbapi.service.consul:8080` and `puppetdb.k8s.syd1.au.unkin.net`), which is what the PR uses. Value is correct, citation doesn't explain the discrepancy. - `cacheFor`/cache-key logic, error-path content-type, and every other header are otherwise correctly untouched and complete: no fact-bearing route stayed cached, and `/nodes` is the only route the cache still covers. Confirmed against a live `/nodes` record that it carries only report/run-lifecycle fields (certname, deactivated/expired, catalog/facts/report timestamps, environments, `latest_report_*`) and no fact key/value data, so a ≤30s-stale `/nodes` read cannot resurface a stale fact — the "report caching" reading is sound. That staleness window on `latest_report_status`/`deactivated`/`expired` already existed pre-PR (nodes was already cached), so it's not a new regression. - Reverting the `cacheFor` switch + header/content-type code on the PR branch reproduces failures in 6 tests (`TestHandler_CachedResponsesCarryNoCacheHeaders`, `TestHandler_FactPathsAreNeverCached`, `TestHandler_FactResponsesCarryNoCacheHeaders`, `TestHandler_SuccessContentTypeMatchesPuppetDB`, `TestHandler_FactsBySourceNameValuesFilterOneFetch`, `TestHandler_FactRoutesAreNotCached`) — close to the claimed five and confirms they aren't vacuous. The deleted fact+cache-interaction tests (source attribution, per-key gating, suppression-survives-hit, stale drilldown filtering) have live equivalents already in the untouched `source_test.go`/`factroutes_test.go`, so coverage moved rather than disappeared. - `go test -race ./...`, `golangci-lint run ./...`, and `pre-commit run --all-files` all pass clean on the PR branch; e2e files still compile under `-tags e2e`. - PR body meets conventions (why + present-tense how, 6 lines, 476 chars).

unkin-agent referenced this pull request

2026-09-13 14:04:33 +10:00

Replay every unanimous upstream status, not just 4xx #27

unkin-agent added 1 commit 2026-09-13 14:54:14 +10:00

Merge main into benvin/drop-fact-caching

ci/woodpecker/pr/build Pipeline was successful

Details

ci/woodpecker/pr/test Pipeline was successful

Details

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

Details

5c9d1e9055

Both sides added jsonContentType; keep one. The no-entry 502 test kills the
backends outright, since main replays a unanimous 500, and the cached
content-type test moves to /nodes, the only cached path left.

benvin merged commit 2652778306 into main

2026-09-13 15:04:26 +10:00

benvin deleted branch benvin/drop-fact-caching

2026-09-13 15:04:27 +10:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: unkin/pdbmux#26