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

Merged

benvin merged 3 commits from benvin/replay-upstream-errors into main 2026-09-13 14:31:56 +10:00

Conversation 2 Commits 3 Files Changed 13

+710 -134

unkin-agent commented 2026-09-13 13:35:38 +10:00

Member

Copy Link

Copy Source

openvoxdb answers the same malformed query with 400 on /nodes but 500 on /facts, and /metrics with a flat 403, so replaying only 4xx made behaviour depend on the route; meta and metrics bypassed the replay entirely.

• Replay any status from 400 up that every backend agreed on, with its own body and content type; 502 stays for disagreement or silence.
• Route /pdb/meta, /metrics and the pass-through path through that same rule.
• Serve the owner's own answer on the per-certname routes, where a non-owner's 404 is not a dissent; fall back to whoever answered when the owner is unreachable.
• Count a unanimous 5xx as a failed round and let it take a stale cache entry; only a unanimous 4xx stays exempt.
• Answer successful queries with openvoxdb's application/json;charset=utf-8.

openvoxdb answers the same malformed query with 400 on `/nodes` but 500 on `/facts`, and `/metrics` with a flat 403, so replaying only 4xx made behaviour depend on the route; meta and metrics bypassed the replay entirely. - Replay any status from 400 up that **every** backend agreed on, with its own body and content type; 502 stays for disagreement or silence. - Route `/pdb/meta`, `/metrics` and the pass-through path through that same rule. - Serve the owner's own answer on the per-certname routes, where a non-owner's 404 is not a dissent; fall back to whoever answered when the owner is unreachable. - Count a unanimous 5xx as a failed round and let it take a stale cache entry; only a unanimous 4xx stays exempt. - Answer successful queries with openvoxdb's `application/json;charset=utf-8`.

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

Replay every unanimous upstream status, not just 4xx ... 121bfacc2f

openvoxdb does not reserve 5xx for its own faults: the same malformed
query is a 400 on /nodes and a 500 on /facts, and /metrics answers a flat
403, so a 4xx-only replay rule made pdbmux's behaviour depend on the
route. The meta and metrics handlers held their own copy of the gateway
error and bypassed the replay entirely.

- Replay any status from 400 up that every backend agreed on, with the
  backend's own body and content type.
- Keep 502 for backends disagreeing on the status, or a backend that
  answered nothing at all.
- Route /pdb/meta, /metrics and the pass-through path through the same
  rule as the merged query handlers.
- Count a unanimous 5xx as a failed round and let it fall back to a stale
  cache entry; only a unanimous 4xx stays exempt from both.
- Answer successful queries with openvoxdb's application/json;charset=utf-8.

unkin-agent commented 2026-09-13 14:04:33 +10:00

Author

Member

Copy Link

Copy Source

Review findings.

Blocking

1. serveByOwner/proxyOrdered regression on the per-certname owner routes (/pdb/query/v4/{nodes,factsets,catalogs}/<certname>[...]). ownerFirst still returns all backends with the owner first; proxyOrdered still tries them in order and short-circuits on the first 2xx (unchanged). But when nobody 2xx's, it now needs upstreamOutcome/unanimity across all of them instead of replaying whichever answered first (the owner). That breaks the one thing this route exists for — "answers a path keyed on one certname from the backend that owns that certname" (certname.go:42-52) — whenever the owner's real answer differs from the non-owner's incidental one. Reproduced on this branch: owner backend faults with a real 500 ("boom"), non-owner legitimately doesn't hold the certname (its own 404) → pre-PR replayed the owner's 500 verbatim; this branch returns a generic 502 all backends failed, discarding the owner's actual answer. Plausible in the live migration topology (a node present on only one backend, which is having a transient issue). Untested: the new TestProxyUnmerged_ReplayRule only covers the plain pass-through route (resourcesPath, configured order), not serveByOwner/certname routes — no test pins the owner-precedence behavior against this rule change.

Non-blocking

• clientRefusal() is a flat status < 500, so a unanimous 403 (previously excluded, treated as a backend-side fault) is now fully exempt from the degraded-round counter and stale fallback. Checked live: today's unanimous 403 on /metrics/v2/list is openvoxdb's own metrics-lockdown policy on both backends, not a TLS/cert issue, so exempting it is correct for this deployment. The risk is only if pdbmux ever authenticates to backends with a client cert that can fail — that would now masquerade as "query answered, estate's own 4xx" rather than a degraded round. Worth a code comment now that the old cert-vs-data rationale (previously in clientShaped's doc) was dropped rather than carried into clientRefusal.
• "puppetboard's /query view is the only real behavioural delta" undersells it. Checked voxpupuli/puppetboard: every other view (nodes, facts, catalogs, metrics, index, radiator, reports, dailychart) uses plain get_or_abort, which does abort(e.response.status_code) for any HTTPError — so those pages will now show Flask's default 403/404/429/500/503 page instead of always 502 for the newly-widened cases, app-wide, not just on /query. Probably desirable (matches "indistinguishable from PuppetDB"), but it's more than one view. Separately, "no retry behaviour changes anywhere" checked out: neither pypuppetdb (api/base.py, bare requests.Session(), unconditional raise_for_status(), no retry adapter) nor puppetboard has any status-based retry logic to begin with.
• jsonContentType's comment cites openvoxdb's http.clj:55-66 (simple-utf8-ctype?) to justify the no-space spelling, but that function only affects Accept-header negotiation, not response serialization — and openvoxdb's actual rr/content-type call (both main and the 8.9.0 tag) passes "application/json; charset=utf-8" with a space. Live-checked anyway: both real backends and current pdbmux answer with no space on the wire (application/json;charset=utf-8), so the chosen constant is empirically correct — the citation just doesn't explain why.
• Verified the central motivating claim live against both backends: ["=","name"] on /nodes → unanimous 400 "= requires exactly two arguments"; on /facts → unanimous 500 with an empty body. Current production pdbmux already replays the first correctly but returns 502 all backends failed for the second — a live, reproducible instance of exactly the inconsistency this PR fixes. engine.clj:2988-2996's unguarded (nth user-query-clause 2) and query_eng.clj:571-583's catch list (IllegalArgumentException, ExceptionInfo with :kind in #{:puppetlabs.puppetdb.query/invalid ::dr/unrecognized-ast-syntax} → 400 via http/error-response, anything else uncaught → 500) both check out against openvoxdb source.
• Edge cases in the unanimity rule: zero backends configured doesn't vacuously replay (empty slice never enters the loop, falls to errAllBackendsFailed/502) — correct, covered by the existing "no backends" case. A backend excluded by health gating leaves the survivor's answer "unanimous" by itself, but that's inherent to how fanOut/liveBackends already worked pre-PR (unanimity has always been computed over the queried set, not the configured set) — this PR just extends it to 5xx, not a new risk.
• Reverted the substantive rule (reconstructed the old 4xx-minus-403/404/408/429 replayableStatus) and reran: every new "unanimous status replayed" test fails as expected, so none are vacuous. go test -race ./..., golangci-lint run ./..., and pre-commit run --all-files all pass clean on the branch.
• PR #26 (benvin/drop-fact-caching) also touches server.go, cache_test.go, upstream_test.go and README.md — flagging the merge-overlap risk per the review brief, not fixing it here.
• Body meets conventions (why + present-tense how, 6 lines, 737 chars, no AI/session/person references).

Review findings. **Blocking** 1. `serveByOwner`/`proxyOrdered` regression on the per-certname owner routes (`/pdb/query/v4/{nodes,factsets,catalogs}/<certname>[...]`). `ownerFirst` still returns *all* backends with the owner first; `proxyOrdered` still tries them in order and short-circuits on the first 2xx (unchanged). But when nobody 2xx's, it now needs `upstreamOutcome`/unanimity across *all* of them instead of replaying whichever answered first (the owner). That breaks the one thing this route exists for — "answers a path keyed on one certname from the backend that owns that certname" (certname.go:42-52) — whenever the owner's real answer differs from the non-owner's incidental one. Reproduced on this branch: owner backend faults with a real 500 ("boom"), non-owner legitimately doesn't hold the certname (its own 404) → pre-PR replayed the owner's 500 verbatim; this branch returns a generic `502 all backends failed`, discarding the owner's actual answer. Plausible in the live migration topology (a node present on only one backend, which is having a transient issue). Untested: the new `TestProxyUnmerged_ReplayRule` only covers the plain pass-through route (`resourcesPath`, configured order), not `serveByOwner`/certname routes — no test pins the owner-precedence behavior against this rule change. **Non-blocking** - `clientRefusal()` is a flat `status < 500`, so a unanimous 403 (previously excluded, treated as a backend-side fault) is now fully exempt from the degraded-round counter and stale fallback. Checked live: today's unanimous 403 on `/metrics/v2/list` is openvoxdb's own metrics-lockdown policy on both backends, not a TLS/cert issue, so exempting it is correct for this deployment. The risk is only if pdbmux ever authenticates to backends with a client cert that can fail — that would now masquerade as "query answered, estate's own 4xx" rather than a degraded round. Worth a code comment now that the old cert-vs-data rationale (previously in `clientShaped`'s doc) was dropped rather than carried into `clientRefusal`. - "puppetboard's `/query` view is the only real behavioural delta" undersells it. Checked voxpupuli/puppetboard: every other view (nodes, facts, catalogs, metrics, index, radiator, reports, dailychart) uses plain `get_or_abort`, which does `abort(e.response.status_code)` for *any* HTTPError — so those pages will now show Flask's default 403/404/429/500/503 page instead of always 502 for the newly-widened cases, app-wide, not just on `/query`. Probably desirable (matches "indistinguishable from PuppetDB"), but it's more than one view. Separately, "no retry behaviour changes anywhere" checked out: neither pypuppetdb (`api/base.py`, bare `requests.Session()`, unconditional `raise_for_status()`, no retry adapter) nor puppetboard has any status-based retry logic to begin with. - `jsonContentType`'s comment cites openvoxdb's `http.clj:55-66` (`simple-utf8-ctype?`) to justify the no-space spelling, but that function only affects Accept-header negotiation, not response serialization — and openvoxdb's actual `rr/content-type` call (both `main` and the 8.9.0 tag) passes `"application/json; charset=utf-8"` *with* a space. Live-checked anyway: both real backends and current pdbmux answer with no space on the wire (`application/json;charset=utf-8`), so the chosen constant is empirically correct — the citation just doesn't explain why. - Verified the central motivating claim live against both backends: `["=","name"]` on `/nodes` → unanimous 400 "= requires exactly two arguments"; on `/facts` → unanimous 500 with an empty body. Current production pdbmux already replays the first correctly but returns `502 all backends failed` for the second — a live, reproducible instance of exactly the inconsistency this PR fixes. `engine.clj:2988-2996`'s unguarded `(nth user-query-clause 2)` and `query_eng.clj:571-583`'s catch list (`IllegalArgumentException`, `ExceptionInfo` with `:kind` in `#{:puppetlabs.puppetdb.query/invalid ::dr/unrecognized-ast-syntax}` → 400 via `http/error-response`, anything else uncaught → 500) both check out against openvoxdb source. - Edge cases in the unanimity rule: zero backends configured doesn't vacuously replay (empty slice never enters the loop, falls to `errAllBackendsFailed`/502) — correct, covered by the existing "no backends" case. A backend excluded by health gating leaves the survivor's answer "unanimous" by itself, but that's inherent to how `fanOut`/`liveBackends` already worked pre-PR (unanimity has always been computed over the queried set, not the configured set) — this PR just extends it to 5xx, not a new risk. - Reverted the substantive rule (reconstructed the old 4xx-minus-403/404/408/429 `replayableStatus`) and reran: every new "unanimous status replayed" test fails as expected, so none are vacuous. `go test -race ./...`, `golangci-lint run ./...`, and `pre-commit run --all-files` all pass clean on the branch. - PR #26 (`benvin/drop-fact-caching`) also touches `server.go`, `cache_test.go`, `upstream_test.go` and `README.md` — flagging the merge-overlap risk per the review brief, not fixing it here. - Body meets conventions (why + present-tense how, 6 lines, 737 chars, no AI/session/person references).

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

Serve the owner's own answer on the per-certname routes ... 72380f27e6

Unanimity is the right rule for a fan-out of peers, but the per-certname
routes are not one: a backend that does not hold the certname answers 404
to say so, not to disagree, so requiring it to agree turned the owner's
real 500 into a 502 that described neither backend.

- Add askOrder, which says whether a set of backends was asked as peers
  or owner-first, and resolve each round's replies under its own rule.
- Serve the first backend that answered on owner-routed paths, so an
  unreachable owner still falls back rather than collapsing to 502.
- Keep unanimity for the merged, meta, metrics and pass-through routes.
- Cover the owner routes: owner errors against a non-owner 404, both
  erroring differently, an unreachable owner, and a non-owner error
  behind the owner's 200.
- Record what clientRefusal's 4xx exemption assumes about client certs.

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

Say that owner-first covers a certname with no resolved owner 85c9088293

benvin merged commit 3bea2e9b71 into main 2026-09-13 14:31:56 +10:00

benvin deleted branch benvin/replay-upstream-errors 2026-09-13 14:31:57 +10:00

unkin-agent commented 2026-09-13 18:40:49 +10:00

Author

Member

Copy Link

Copy Source

Targeted review: 72380f2, 85c9088

Verdict: correct, no blocking issues. Both commits do what they claim; the fix resolves the owner-masking bug described in the PR without touching backend selection.

Verified

• ownerOutcome correctness. Walked all combinations by hand and confirmed with the existing sequential proxyOrdered loop (unchanged shape, only the askOrder param is new): owner 2xx short-circuits immediately (never reaches outcome); owner 4xx/5xx wins over a non-owner's differing status because ownerOutcome scans replies in ask order and replies[0] is always the owner's slot; owner silent (nil entry) correctly falls through to the first non-nil replayable fallback reply; all-silent and owner-silent+fallback-silent both correctly 502. A 3xx from the owner is (consistent with peerOutcome/replayableStatus) not replayable and falls through like a silent reply — unchanged pre-existing behavior, not a regression.
• Route split. proxyUnmerged→askPeers, serveByOwner→askOwnerFirst; no other fan-out route was missed (meta.go, server.go:538 both correctly renamed to peerOutcome, no leftover references to the deleted upstreamOutcome).
• serveFirstHolder left on peerOutcome: agree with the author's call. Unlike certname routes, there's no independent oracle (freshness map) naming which backend should hold a given report hash before asking — "owner" there can only be inferred post-hoc from who actually answered, so there's no principled way to prefer one backend's error over another's the way ownerFirst() can. Worth a one-line comment on serveFirstHolder noting this was considered and rejected, so it doesn't look like an oversight next time someone reads it.
• Unresolved-owner fallback to configured order: correct, and confirmed by diff that 85c9088 is comment-only (no lines outside comments changed).
• No selection regression: ownerFirst() body is byte-for-byte unchanged from eeb4405; the diff only threads askOwnerFirst through the existing call. Confirmed live against puppetdb.k8s.syd1.au.unkin.net and pdbmux.k8s.syd1.au.unkin.net that the 404 shape ("No information is known about node <certname>") ownerFirst/serveByOwner rely on to treat a non-owner's reply as "not mine" matches production.
• Tests: TestCertnameRoutes_OwnerAnswerWins — ran it against 121bfac (pre-fix) with 3/5 subtests failing exactly as claimed (owner-errors-vs-404, owner-and-non-owner-error-differently, owner-unreachable-fallback-answers all got a 502 instead of the owner's/fallback's real status), and against 5207a79 (pre-PR) with all 5 passing — genuinely proves regression, not a new requirement. go test -race ./..., go vet, gofmt -l, golangci-lint run, and pre-commit run --all-files are all clean at head. The voxBackend extension (perNodeStatus/perNodeBody/perNodeDead) is opt-in (guarded by isCertnameRoute + non-zero checks) and doesn't touch the pre-existing /nodes and /factsets fake handlers, so it doesn't weaken existing coverage.

Non-blocking

1. Confirmed: TestCertnameRoutes_OneBackendDown's stated rationale is wrong. a.fail = true intercepts every path including /nodes, so a is excluded from freshnessMap's alive set entirely and buildFreshness attributes h1.example.net to b directly — b is the freshness-resolved owner from the start, not a fallback. Combined with proxyOrdered's early-return-on-first-success, a is never even contacted. The test still passes and still proves something real (fan-out survives one dead backend), just not "the fallback is exercised only if a is skipped" as its comment claims — no fallback path is exercised here at all; that's what the new owner unreachable, fallback answers subtest in TestCertnameRoutes_OwnerAnswerWins (using perNodeDead, which correctly keeps a resolvable as owner via /nodes while only killing its per-certname reply) actually covers. Checked the other 16 call sites of .fail = true/.fail, b.fail across the suite — none of the others make an ownership/freshness claim in their comments, so this false premise is isolated to this one pre-existing test (not touched by these two commits). Pre-existing, so not this PR's fault, but worth a follow-up to fix the comment or make it actually exercise fallback.
2. The new clientRefusal comment asserts the /metrics/v2/list 403 is "that backend's own policy rather than an authentication failure" without a source citation — that endpoint appears to be served by trapperkeeper-metrics rather than anything in OpenVoxProject/openvoxdb itself, so I could not confirm it against openvoxdb source (no file:line to check). Doesn't affect correctness of the code, just an unsourced assumption in a comment that reads like the other sourced ones.

PR body

Compliant: 800/800 chars, 7 lines, present-tense bullets, final-state only, no review history or session/person references.

## Targeted review: 72380f2, 85c9088 **Verdict: correct, no blocking issues.** Both commits do what they claim; the fix resolves the owner-masking bug described in the PR without touching backend selection. ### Verified - **ownerOutcome correctness.** Walked all combinations by hand and confirmed with the existing sequential `proxyOrdered` loop (unchanged shape, only the `askOrder` param is new): owner 2xx short-circuits immediately (never reaches `outcome`); owner 4xx/5xx wins over a non-owner's differing status because `ownerOutcome` scans `replies` in ask order and `replies[0]` is always the owner's slot; owner silent (nil entry) correctly falls through to the first non-nil replayable fallback reply; all-silent and owner-silent+fallback-silent both correctly 502. A 3xx from the owner is (consistent with `peerOutcome`/`replayableStatus`) not replayable and falls through like a silent reply — unchanged pre-existing behavior, not a regression. - **Route split.** `proxyUnmerged`→`askPeers`, `serveByOwner`→`askOwnerFirst`; no other fan-out route was missed (`meta.go`, `server.go:538` both correctly renamed to `peerOutcome`, no leftover references to the deleted `upstreamOutcome`). - **serveFirstHolder left on peerOutcome:** agree with the author's call. Unlike certname routes, there's no independent oracle (freshness map) naming which backend *should* hold a given report hash before asking — "owner" there can only be inferred post-hoc from who actually answered, so there's no principled way to prefer one backend's error over another's the way `ownerFirst()` can. Worth a one-line comment on `serveFirstHolder` noting this was considered and rejected, so it doesn't look like an oversight next time someone reads it. - **Unresolved-owner fallback to configured order:** correct, and confirmed by diff that 85c9088 is comment-only (no lines outside comments changed). - **No selection regression:** `ownerFirst()` body is byte-for-byte unchanged from `eeb4405`; the diff only threads `askOwnerFirst` through the existing call. Confirmed live against `puppetdb.k8s.syd1.au.unkin.net` and `pdbmux.k8s.syd1.au.unkin.net` that the 404 shape (`"No information is known about node <certname>"`) `ownerFirst`/`serveByOwner` rely on to treat a non-owner's reply as "not mine" matches production. - **Tests:** `TestCertnameRoutes_OwnerAnswerWins` — ran it against `121bfac` (pre-fix) with 3/5 subtests failing exactly as claimed (owner-errors-vs-404, owner-and-non-owner-error-differently, owner-unreachable-fallback-answers all got a 502 instead of the owner's/fallback's real status), and against `5207a79` (pre-PR) with all 5 passing — genuinely proves regression, not a new requirement. `go test -race ./...`, `go vet`, `gofmt -l`, `golangci-lint run`, and `pre-commit run --all-files` are all clean at head. The `voxBackend` extension (`perNodeStatus`/`perNodeBody`/`perNodeDead`) is opt-in (guarded by `isCertnameRoute` + non-zero checks) and doesn't touch the pre-existing `/nodes` and `/factsets` fake handlers, so it doesn't weaken existing coverage. ### Non-blocking 1. **Confirmed: `TestCertnameRoutes_OneBackendDown`'s stated rationale is wrong.** `a.fail = true` intercepts *every* path including `/nodes`, so `a` is excluded from `freshnessMap`'s `alive` set entirely and `buildFreshness` attributes `h1.example.net` to `b` directly — `b` is the freshness-resolved owner from the start, not a fallback. Combined with `proxyOrdered`'s early-return-on-first-success, `a` is never even contacted. The test still passes and still proves something real (fan-out survives one dead backend), just not "the fallback is exercised only if a is skipped" as its comment claims — no fallback path is exercised here at all; that's what the new `owner unreachable, fallback answers` subtest in `TestCertnameRoutes_OwnerAnswerWins` (using `perNodeDead`, which correctly keeps `a` resolvable as owner via `/nodes` while only killing its per-certname reply) actually covers. Checked the other 16 call sites of `.fail = true`/`.fail, b.fail` across the suite — none of the others make an ownership/freshness claim in their comments, so this false premise is isolated to this one pre-existing test (not touched by these two commits). Pre-existing, so not this PR's fault, but worth a follow-up to fix the comment or make it actually exercise fallback. 2. The new `clientRefusal` comment asserts the `/metrics/v2/list` 403 is "that backend's own policy rather than an authentication failure" without a source citation — that endpoint appears to be served by trapperkeeper-metrics rather than anything in `OpenVoxProject/openvoxdb` itself, so I could not confirm it against openvoxdb source (no file:line to check). Doesn't affect correctness of the code, just an unsourced assumption in a comment that reads like the other sourced ones. ### PR body Compliant: 800/800 chars, 7 lines, present-tense bullets, final-state only, no review history or session/person references.

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

bug

Something is not working

duplicate

This issue or pull request already exists

enhancement

New feature

help wanted

Need some help

invalid

Something is wrong

question

More information is needed

wontfix

This won't be fixed

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

benvin (BenVincent) unkin-agent (Unkin Agent) unkinben (Ben Vincent)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: unkin/pdbmux#27