Gates / frontend (push) Successful in 1m46s
Gates / test (push) Successful in 2m32s
Gates / package (push) Successful in 4m20s
Gates / test-aarch64 (push) Successful in 8m15s
Gates / container (push) Successful in 15s
CI / gates (push) Successful in 30m33s
waiting for a slot now spends the query budget; truncated attempts that expire fault the budget, not the upstream, and are never attributed. admission sweeps in priority order before blocking. forward zones spend read_timeout_ms once across udp, truncation and tcp. adds nxdns_upstream_budget_exhausted_total and a 64-upstream validation limit.
338 lines
18 KiB
Markdown
338 lines
18 KiB
Markdown
# Milestone 37: Upstream failover budget — deadline ownership and honest attribution
|
|
|
|
Make the pool's failover loop own one deadline (admission included), revive the
|
|
standby under primary saturation, and stop blaming endpoints for budget
|
|
exhaustion.
|
|
|
|
## The defect (field-verified on the Pi, 0.0.11)
|
|
|
|
With defaults attempt=2500 ms / total=5000 ms and two upstreams:
|
|
|
|
1. `Pool.exchangeLoopLen` (pool.zig:277) never computes remaining time. The
|
|
semaphore wait consumes the total budget invisibly: under a traffic burst
|
|
the primary's slots saturate, a new request queues behind them while the
|
|
standby sits idle with free slots, and the outer `raceWithin(total)` cancels
|
|
whatever finally runs. 155 requests died at the 5 s cap; 706 of 717
|
|
SERVFAILs came from one bursty client. On an idle pool a fast standby works
|
|
today — the queue is the killer, not the timer arithmetic alone.
|
|
2. The reporting contradicts itself. `selected` is written before the attempt
|
|
runs (pool.zig:348), so the query log blamed the standby on 279 rows;
|
|
cancelled attempts are excluded from health (pool.zig:363), so its counters
|
|
read 0/0. Nothing records "the request exhausted its own budget."
|
|
|
|
Design reviewed and converged with Codex (thread of 2026-08-27). Defaults do
|
|
not change.
|
|
|
|
## Sessions
|
|
|
|
Three, strictly sequential: A (transport vocabulary) → B (pool) → C
|
|
(handler, forward client, surfaces).
|
|
|
|
---
|
|
|
|
## Session A: transport vocabulary
|
|
|
|
### A.1 `error.BudgetExhausted` and a fourth group (src/upstream/transport.zig)
|
|
|
|
- Add `BudgetExhausted` to `ExchangeError`.
|
|
- Add `.budget_exhausted` to `Group`; `group()` maps the new error to it. The
|
|
switch stays exhaustive with no `else` — every switch over `Group` breaks at
|
|
compile time until it handles the new group, which is the point. The
|
|
production switches are `pool.zig` (:353), `handler.zig` (:677, :735) and
|
|
`forward_client.zig` (:124). Session A owns a mechanical placeholder arm at
|
|
each (pool and forward_client: propagate the error without recording
|
|
health; handler: `=> ctx.servFail()`), plus any test switches the compiler
|
|
flags, so A's `zig build test` passes. B and C then own the real behavior
|
|
at their sites. (health.zig has no `group` call; the doh/dot occurrences
|
|
are tests, not switches.)
|
|
|
|
### A.2 Timer-origin race (src/upstream/transport.zig)
|
|
|
|
`raceWithin` collapses "the expiry task won" and "the raced operation itself
|
|
returned error.Timeout" into one `error.Timeout`. Add:
|
|
|
|
```zig
|
|
pub const RaceOutcome = enum { completed, expired };
|
|
|
|
pub fn raceUntilTagged(
|
|
io: std.Io,
|
|
expiry_at: std.Io.Clock.Timestamp,
|
|
outcome: *RaceOutcome,
|
|
comptime f: anytype,
|
|
args: anytype,
|
|
) ExchangeError!RacedPayload(f)
|
|
```
|
|
|
|
The expiry parameter is an ABSOLUTE timestamp, not a duration: a duration
|
|
computed from `deadline.toDurationFromNow()` and then slept re-anchors at
|
|
"now", drifting past the total deadline and misclassifying a nominally full
|
|
attempt. The pool computes `expiry_at = min(now + attempt, total_deadline)`
|
|
and passes the timestamp. On the expiry side the function sets `outcome.* =
|
|
.expired` and returns `error.Timeout`; on completion it sets `.completed` and
|
|
returns the raced result (which may itself be `error.Timeout` from the leaf —
|
|
that is a completed peer timeout, not an expiry). `raceWithin` keeps its
|
|
public API and behavior but delegates to the same internal harness (compute
|
|
the absolute deadline, discard the outcome) — one `Select` harness in the
|
|
file, not two copies.
|
|
|
|
### A.3 Acceptance
|
|
|
|
- [ ] `zig build test` green.
|
|
- [ ] Unit tests: tagged race distinguishes leaf `error.Timeout` (completed)
|
|
from expiry (`expired`); the untagged wrapper is unchanged behavior.
|
|
|
|
---
|
|
|
|
## Session B: the pool (src/upstream/pool.zig)
|
|
|
|
### B.1 One deadline owns the loop
|
|
|
|
- `Pool.exchange` establishes `deadline = std.Io.Timeout{ .duration =
|
|
self.timeouts.total }.toDeadline(io)` (`.awake` clock, as the loop uses
|
|
today) and passes it down. The equal-deadline outer `raceWithin` at
|
|
pool.zig:267 is REMOVED — the loop owns the deadline; two timers aimed at
|
|
the same instant race each other and let the outer cancellation bypass the
|
|
loop's classification. No replacement watchdog (the outer race never bounded
|
|
uncancelable health writes anyway; a later-firing watchdog is scope creep).
|
|
- Every blocking step consumes the deadline:
|
|
- Admission: through the ownership-safe protocol of B.2 — never a bare
|
|
`Semaphore.wait` raced against an expiry. Racing the wait with
|
|
`Select.cancelDiscard` can leak a permit: the wait may have decremented
|
|
the count in the same instant the expiry wins, and the discarded success
|
|
never reaches the caller's release-defer. If no time remains before
|
|
admission, return `error.BudgetExhausted` without waiting.
|
|
- Attempt: raced via `raceUntilTagged` against
|
|
`expiry_at = min(now + timeouts.attempt, total_deadline)`.
|
|
- `total < attempt` is already rejected by validate.zig; `total == attempt`
|
|
(and any admission overhead) simply yields truncated attempts, handled by
|
|
B.3.
|
|
|
|
### B.2 Admission without head-of-line blocking
|
|
|
|
Zig 0.16's `Semaphore` has neither try-acquire nor a timed wait, so the pool
|
|
gains a local admission helper mirroring the standard semaphore's own
|
|
mutex/decrement/condition protocol (never by patching `../zig`, never by
|
|
spinning):
|
|
|
|
- `tryAcquire()` — take a permit if one is immediately available, else fail
|
|
without blocking.
|
|
- `acquireUntil(deadline)` — timed acquisition; returns Acquired (holding a
|
|
permit), Expired (holding none), or `error.Canceled` (holding none).
|
|
`std.Io.Condition` has no timed wait in 0.16, so this is a NEW pool-local
|
|
primitive, specified exactly: state lives under one mutex (permit count +
|
|
waiter bookkeeping); the wait itself may race `Condition.wait` against a
|
|
sleep via `Select`, because a permit is only ever taken under the mutex
|
|
AFTER the race resolves — a discarded wake is a lost notification, not a
|
|
lost permit. To keep that lost notification from stranding another waiter,
|
|
an exiting waiter that may have absorbed a signal (expiry or cancellation
|
|
path) re-signals the condition before returning. Permit conservation is by
|
|
construction: the decrement and the "did I win" decision happen under the
|
|
same mutex. Every take is non-blocking (the mutex's `tryLock`): a lock
|
|
miss reads as "no permit now", and `acquireUntil` re-enters the
|
|
absolute-deadline race on each miss, so contention on the admission mutex
|
|
never carries a call past its deadline (review round 2026-08-27). Tests:
|
|
an expiry/acquisition tie leaves the permit count exact; a cancelled
|
|
waiter holds nothing and a peer waiter still wakes; a contended admission
|
|
mutex does not carry `acquireUntil` past its deadline.
|
|
|
|
Loop semantics — priority means ordering among immediately admissible
|
|
candidates:
|
|
|
|
1. Pass one, first sweep in priority order: `tryAcquire` on each available
|
|
entry; the first immediate success is attempted. A saturated entry is
|
|
skipped while another eligible entry has capacity.
|
|
2. If the attempted entry fails (peer fault), the sweep continues from the
|
|
next entry, still by `tryAcquire`; previously skipped saturated entries
|
|
are re-tried by `tryAcquire` on each subsequent sweep step (a slot may
|
|
have freed).
|
|
3. Only when no eligible entry has an immediate permit does the loop block:
|
|
`acquireUntil(remaining deadline)` on the highest-priority eligible
|
|
entry. Expired → `error.BudgetExhausted`. The no-head-of-line guarantee
|
|
is deliberately scoped to capacity observed during the sweep: once
|
|
blocked, a lower-priority slot freeing does not wake this waiter
|
|
(any-entry wakeups need multi-wait machinery this milestone does not
|
|
buy). Record this bound in the admission helper's doc comment.
|
|
4. Pass two (backoff probing) keeps today's in-order probing but admits
|
|
through the same helper bounded by the remaining deadline — it
|
|
deliberately retains blocking, one entry at a time, because probing a
|
|
backed-off entry is already a last resort.
|
|
5. The post-admission health recheck survives the refactor: after acquiring
|
|
a permit by either path in pass one, re-read health against a fresh
|
|
`now`; an entry that entered backoff while this task waited is released
|
|
(permit returned) and the sweep resumes. The existing regression test
|
|
for this recheck is retained.
|
|
6. Before any attempt starts — immediate admission included — the loop
|
|
re-reads the clock; a deadline already passed returns
|
|
`error.BudgetExhausted` with the permit returned and no endpoint named,
|
|
so an instantly-completing leaf can never manufacture evidence after
|
|
exhaustion (review round 2026-08-27). Test: an exchange whose deadline
|
|
is already gone starts no attempt.
|
|
7. The compiled pool bound (`Pool.max_entries`, 64) is enforced at config
|
|
validation: more than 64 ENABLED upstreams is `TooManyUpstreams` at path
|
|
`upstreams`, so a valid config can never trip the pool assert (review
|
|
round 2026-08-27). Tests: 64 enabled passes, 65 fails, 65 listed with
|
|
64 disabled passes.
|
|
8. Queue accounting keeps its meaning: `queued_total` and
|
|
`queued_seconds_total` count only the blocking `acquireUntil` path,
|
|
recorded whether it ends in acquisition, expiry or cancellation; a
|
|
`tryAcquire` — hit or miss — never counts as queued. Acceptance test
|
|
retained.
|
|
|
|
### B.3 Classification (uses A.2's tagged race)
|
|
|
|
| Attempt outcome | Budget it ran with | Health | `selected` | Loop action |
|
|
| --- | --- | --- | --- | --- |
|
|
| success | any | success | this endpoint | return answer |
|
|
| expiry (`expired`) | full `attempt` | failure | this endpoint | continue failover |
|
|
| expiry (`expired`) | truncated | untouched | unchanged | return `error.BudgetExhausted` |
|
|
| completed peer fault (incl. leaf Timeout) | any, even truncated | failure | this endpoint | continue failover |
|
|
| local_resource | — | untouched | unchanged | return err (as today) |
|
|
| cancellation | — | untouched | unchanged | return `error.Canceled` |
|
|
| wait exhausts deadline | — | untouched | unchanged (null if nothing ran) | return `error.BudgetExhausted` |
|
|
| completed attempt returns `error.BudgetExhausted` (a leaf may emit it once it exists) | any | untouched | unchanged | return it; counter increments once at the outer pool |
|
|
|
|
A truncated expiry is a censored observation: the pool did not grant the
|
|
configured observation interval, so it is evidence about the pool's budget,
|
|
never about the peer. No minimum-attempt floor exists.
|
|
|
|
### B.4 `selected` = last attributable endpoint
|
|
|
|
Assign `selected.*` only in the success row and the two health-recording
|
|
failure rows above — after the attempt completes, not before it starts. This
|
|
attribution rule is POOL-SPECIFIC: leaf clients (DoH, DoT, ForwardClient,
|
|
test fakes) keep their write-before-attempt behavior — they have one
|
|
endpoint and record no health, so "the endpoint I tried" is honest there.
|
|
The `Client.exchange` doc comment in transport.zig is amended to state both
|
|
contracts: implementations may write before each attempt; `Pool` documents
|
|
its stricter last-attributable rule on `Pool.exchange` itself. The pool-level
|
|
tests at pool.zig:748-771 (selected-before-attempt) and :910 invert into the
|
|
new contract's tests; leaf-client tests are untouched.
|
|
|
|
### B.5 The counter
|
|
|
|
`Pool` gains one pool-level counter, `budget_exhausted_total` (atomic u64,
|
|
incremented once per exchange that returns `error.BudgetExhausted`, never per
|
|
endpoint). `Pool.snapshot` returns per-entry rows and cannot carry a
|
|
pool-wide number without duplicating it — so the counter is exposed through a
|
|
separate getter, `Pool.budgetExhaustedTotal()`, and B owns the mechanical
|
|
plumbing at every existing snapshot call site its change touches so B's own
|
|
gate passes before C. No per-stage split (queue vs attempt) —
|
|
fixed-cardinality stage labels are deferred until an operator needs them.
|
|
|
|
### B.6 Acceptance (the decisive regressions first)
|
|
|
|
- [ ] Fast standby at shipped defaults: entry 0 stalls its full attempt
|
|
budget, entry 1 answers instantly, attempt = total/2 → entry 1 answers.
|
|
- [ ] Saturated primary, free standby: entry 0's slots all held by stalled
|
|
exchanges, entry 1 free and fast → entry 1 answers well inside the
|
|
deadline. This is the Pi reproduction; it must FAIL against the current
|
|
code and pass after B.2.
|
|
- [ ] Truncated expiry mutates no health, returns BudgetExhausted, leaves
|
|
`selected` at the last attributable endpoint.
|
|
- [ ] Truncated attempt failing with ConnectionRefused mutates health and
|
|
updates `selected`.
|
|
- [ ] Queue-only exhaustion → BudgetExhausted with `selected == null`.
|
|
- [ ] All-backoff pass two runs under the same deadline.
|
|
- [ ] External cancellation returns Canceled, counts no budget, mutates
|
|
nothing.
|
|
- [ ] `budget_exhausted_total` increments once per exhausted exchange.
|
|
- [ ] Existing invariants hold: cancelled waiter returns its permit; two
|
|
stalling upstreams cost ≤ total, not one budget each.
|
|
|
|
---
|
|
|
|
## Session C: handler, forward client, surfaces
|
|
|
|
### C.1 Handler (src/server/handler.zig)
|
|
|
|
Both `transport.group` switches (:677, :735) gain `.budget_exhausted =>
|
|
ctx.servFail()` — SERVFAIL on the wire, same as peer faults; no rcode fits
|
|
better. No per-query warn log (spam); the counter is the record.
|
|
|
|
### C.2 Forward client (src/local/forward_client.zig)
|
|
|
|
One outer budget for the whole exchange: wrap UDP attempt → truncation
|
|
fallback → TCP in a single tagged race against `read_timeout` (the UDP
|
|
receive's internal deadline and the TCP `raceWithin` at :217 collapse into
|
|
the one outer bound — remove the fresh TCP budget). Every timeout here
|
|
concerns the single configured resolver, so expiry stays `error.Timeout`
|
|
(peer evidence), never BudgetExhausted — the budget/peer distinction is pool
|
|
policy. Test: truncated-UDP-then-stalled-TCP completes or expires within one
|
|
`read_timeout`, not two. Update the doc comment on `read_timeout_ms` in the
|
|
config (validate.zig:360 note and the settings description) to say it bounds
|
|
the whole forward-zone exchange. The key is NOT renamed (anti-requirement).
|
|
|
|
### C.3 Surfaces
|
|
|
|
- Metrics: `nxdns_upstream_budget_exhausted_total` (pool-wide) wherever
|
|
`nxdns_upstream_*` counters render, read via `Pool.budgetExhaustedTotal()`.
|
|
- `/api/health` is NOT changed (decision, not omission): the counter is an
|
|
operator metric, it never changes health status, and adding it to the API
|
|
would drag openapi.yaml, admin types, fixtures, contract samples and the
|
|
admin gates into a milestone that owes them nothing. Metrics only.
|
|
- docs: `docs/reference/configuration.md` — every statement describing
|
|
`read_timeout_ms` as a per-read or per-attempt bound is rewritten to the
|
|
whole-exchange contract; the upstream timeouts section gains one paragraph
|
|
on budget semantics (deadline, truncation, attribution). The stale contract
|
|
text in `src/config/model.zig` (the setting's doc comment) and the note at
|
|
`src/config/validate.zig:360` are updated to match.
|
|
|
|
### C.4 Acceptance
|
|
|
|
- [ ] `zig build test` and `zig build test -Dintegration` green.
|
|
- [ ] Metrics test covers the new counter's rendering.
|
|
- [ ] Forward-client single-budget test per C.2.
|
|
|
|
---
|
|
|
|
## File ownership
|
|
|
|
- A: `src/upstream/transport.zig`, plus mechanical placeholder arms at the
|
|
broken `Group` switches (`src/upstream/pool.zig`, `src/server/handler.zig`,
|
|
`src/local/forward_client.zig`, test switches the compiler flags) —
|
|
sequential ownership, A runs alone; C takes forward_client.zig and
|
|
handler.zig over later in sequence.
|
|
- B: `src/upstream/pool.zig` (+ the `Client.exchange` doc contract in
|
|
transport.zig and snapshot-caller plumbing for the new getter — B runs
|
|
alone after A).
|
|
- C: `src/server/handler.zig`, `src/local/forward_client.zig`, the metrics
|
|
rendering files, `src/config/model.zig` + `src/config/validate.zig` doc
|
|
text, `docs/reference/configuration.md`.
|
|
- Orchestrator: CHANGELOG.md.
|
|
|
|
## Acceptance criteria (milestone complete)
|
|
|
|
- [ ] All session criteria; both suites green; `zig fmt --check` clean.
|
|
- [x] The saturated-primary regression demonstrably fails on pre-milestone
|
|
code and passes after. Evidence (orchestrator-run mutation check,
|
|
2026-08-27): with the pass-one `tryAcquire` sweep disabled in
|
|
`Pool.admit` — restoring pre-fix head-of-line blocking — the test
|
|
"a saturated primary defers to a standby that has capacity" fails with
|
|
`error.BudgetExhausted` out of the blocking admission path, and four
|
|
queue-accounting/attribution tests fail with it (5 failed, seed
|
|
0x91ce5df5). Sweep restored: 30/30 steps, 1892/2067 passed, 0 failed.
|
|
- [ ] Changelog: failover now works under primary saturation; SERVFAILs from
|
|
budget exhaustion are counted, not blamed on an upstream; for
|
|
upstream-pool queries the query-log `upstream` field now names only
|
|
endpoints whose outcome was recorded (may be null) — forward-zone
|
|
queries keep naming their single configured resolver as before.
|
|
|
|
## Anti-requirements
|
|
|
|
- No default timeout changes.
|
|
- No parallel/racing fan-out to multiple upstreams. Priority failover stays,
|
|
with priority defined as ordering among immediately admissible candidates
|
|
(B.2) — a saturated higher-priority entry defers to an admissible
|
|
lower-priority one; it does not outrank an idle standby by blocking on it.
|
|
- No `/api/health` or admin changes; the counter surfaces in metrics only.
|
|
- No patching of the vendored/system Zig stdlib; the admission helper is
|
|
pool-local.
|
|
- No rename of `upstream.read_timeout_ms` (doc fix only).
|
|
- No minimum-attempt floor constant.
|
|
- No per-stage split of the budget counter; no per-query budget log lines.
|
|
- No watchdog replacing the removed outer race.
|
|
- No fake upstream identity (e.g. "budget") in the query log; no new
|
|
query-log column.
|
|
- No changes to backoff policy, slot counts, or health scoring beyond the
|
|
attribution rules above.
|