upstream: a diagnostics episode follows health, and a peer fault carries its cause
Gates / frontend (push) Successful in 2m5s
Gates / test (push) Successful in 2m43s
Gates / test-aarch64 (push) Successful in 8m19s
Gates / package (push) Successful in 4m21s
Gates / container (push) Successful in 13s
CI / gates (push) Successful in 30m43s
Gates / frontend (push) Successful in 2m5s
Gates / test (push) Successful in 2m43s
Gates / test-aarch64 (push) Successful in 8m19s
Gates / package (push) Successful in 4m21s
Gates / container (push) Successful in 13s
CI / gates (push) Successful in 30m43s
This commit is contained in:
@@ -810,3 +810,39 @@ The implementation matches the spec with these review-driven refinements (three
|
||||
- `dot_client.zig`: handshake `ReadFailed`/`WriteFailed` unwrap the stream's stored cause through `transport.mapLocal` first, and certificate-bundle `OutOfMemory` is a local resource, not a peer fault.
|
||||
- `transport.zig`: `Endpoint.parse` rejects userinfo/query/fragment delimiters (`@`, `?`, `#`) in the authority, and `?`/`#` in a DoH path.
|
||||
- `dot_client.zig` (follow-up, tls_name commit): `DotClient.init` takes a `tls_name`; it is the SNI and certificate-verification name, while the dial target stays `endpoint.host`. Empty keeps the endpoint host, which is the behavior described above. The same follow-up fixed a send bug this file had from the start, invisible until a DoT handshake first succeeded: `tls.Client.flush` only encrypts into the socket writer's buffer and never flushes it, so the query never left the process and the peer eventually closed the connection (`ReceiveFailed`/`EndOfStream`). `TlsStream.flush` now does both flushes and `exchange` calls it; a hermetic loopback test in `tls_client_integration_test.zig` covers it.
|
||||
|
||||
## Addendum (2026-09-12): an upstream episode follows health, and a peer fault carries its cause
|
||||
|
||||
Observed on the Pi over three days (nxdns 0.0.17 to 0.0.19): 20 `upstream.exchange` warning episodes, 17 with one occurrence, 15 resolved within ten seconds; since the 0.0.20 restart 1229 successes, 0 failures, 401 silent stale-session redials. `Pool.recordFailure` reports an episode on every failed exchange and `recordSuccess` resolves it on the next success, while `health.State` trips backoff only at `failure_threshold` consecutive failures. Two rules over one failure stream, and the Diagnostics page shows the looser one. Second gap: the episode detail is `upstream 'tls://1.1.1.1:853' failed: SendFailed`; the DoT client unwraps the concrete cause into `Transact.Failure.cause` and then returns a bare `PeerFault` error to the pool, so the cause is lost before anything records it. Codex reviewed the design (thread 01a09648) and its eight findings are folded in below.
|
||||
|
||||
### 1. Health owns "failing"; the episode is a projection of its transitions
|
||||
|
||||
- `health.State.recordFailure(at, fault, cfg, rand)` and `recordSuccess(at, cfg)` return an `Effect`: `{ revision: u64, state: union(enum) { clear, tripped: Fault } }`. `revision` is a per-state counter incremented by every mutation. The effect is the complete desired state of the episode after the mutation, never a transition: `tripped` when `consecutive_failures >= cfg.failure_threshold` (the predicate line 142 already uses, exposed as `tripped(cfg)`; not `available()`, which turns true when a backoff expires before recovery is proven), `clear` otherwise. The fault in `tripped` is the effective last fault the state holds after the mutation, not the incoming one: a stale failure (case 1 at `recordFailure`) never displaces a newer cause, so the episode detail cannot regress to an older one. A tagged union, not an action enum with a payload field, so a report without a fault is unrepresentable.
|
||||
- The pool projects every effect onto the store: `tripped` reports, `clear` resolves. A resolve when nothing is open issues no SQL (`storage/events.zig`), so the steady state costs nothing. Because every effect carries the whole desired state, last-writer-wins by revision is correct: the pool applies effects per entry in revision order under a separate `diagnostics_mutex` with a per-entry `applied_revision`, and drops an effect whose revision is below the applied one. Codex's reordering case (a failure trips health, a newer success clears it and its resolve reaches the store first, then the delayed failure arrives) is dropped instead of opening an episode nothing will resolve; and the converse cases, where a `none` transition would have overtaken a required report or resolve, cannot occur because there is no `none`. Health mutates under `Pool.mutex` as today and the effect is computed there; the store call runs after that mutex is released. Lock order is pool mutex, then nothing; diagnostics mutex, then store mutex. No task holds the pool mutex while it takes either of the other two.
|
||||
- Projection also reconciles a latched store failure for what an entry owns: a `clear` that failed to write is retried by the next `clear` the entry projects, and a `tripped` by the next failure. What no entry owns is handled by the reconciliation points below.
|
||||
- A success that leaves the state tripped (a stale success behind a newer failure, health case 1) projects nothing: `recordSuccess` returns `?Effect`, null in that case. The card is already open from the failure that tripped it, a report on a success would count a successful exchange as an occurrence, and a no-op that advanced the revision would reintroduce the dropped-report hole. Such a success does not touch `applied_revision`.
|
||||
- Reconciliation at the two points revision order cannot reach. (1) At boot, once the first generation is built: `store.resolveExcept(.upstream_exchange, kept = the enabled urls of the pool)` closes persisted episodes for upstreams that are gone, disabled or failed to build, and `Pool.reconcile(io)` projects every entry's current effect, which on a fresh pool resolves the rest. A re-asserted `tripped` must not count as an occurrence: the store gains `ensureOpen`, which opens the episode with the detail when none is active and otherwise leaves `occurrences` and `last_seen` untouched; `reconcile` uses it, a recorded failure keeps using `report`. (2) At every retirement of a displaced generation, both the pinned path (`Owner.release` at `refs == 0`, the point after which no exchange of it can still project) and the idle path (`Owner.replace` handing a zero-ref generation back to the caller), the same two calls run against the current generation. One owner function does the retire-and-reconcile and both paths call it, so a third retirement site cannot forget it. A removed or disabled upstream's episode is closed at the next reconciliation point; if the store had latched a failure at that moment, the next reconciliation point retries it. Before this addendum such an episode was never closed at all. `resolveExcept` is the right call for this code: the pool is the only reporter of `upstream_exchange`, unlike `configuration.load`, whose scoped rule in owner.zig stays. The previous "one exchange of lag" claim is withdrawn; the retirement point is exact.
|
||||
- Occurrences are the count of applied reports. Under concurrent reordering an older report behind a newer one is dropped, so the count can undercount; the health counters on `/metrics` are exact. Stated, not fixed: exact occurrence counts would need the store to accept out-of-order increments, and nobody reads the count as a metric.
|
||||
- The store folds a repeated report into the open episode (`occurrences`, `last_seen`, detail), so failures during backoff raise counts on one card. A warning card now means exactly what the pool means: it stopped trusting this endpoint.
|
||||
- Boundary, stated on purpose: an endpoint failing every other exchange never trips, because every newest success clears the count, so it opens no episode and `/api/health` stays `ok`. The rolling success rate on `/metrics` shows it. Diagnostics is for episodes, not chronic rates; a second predicate for that is not added.
|
||||
- No new configuration. The threshold that exists is the threshold, and it is at least 1: `health.Config` is validated where it is constructed (a comptime check on the default; if it is user-configurable, the config loader refuses 0), because a tripped state with no recorded failure has no fault to report.
|
||||
|
||||
### 2. A peer fault is a value between the leaf client and the pool
|
||||
|
||||
- `transport.zig` gains `Fault = struct { kind: PeerFault, cause: anyerror }`, `Outcome = union(enum) { reply: []u8, fault: Fault }`, and a leaf interface `Leaf` with `exchangeFn(ptr, io, query, response_buf) (LocalResource || Cancellation)!Outcome`. `kind` is the taxonomy as today; `cause` is the concrete unwrapped error (`BrokenPipe`, `ConnectionResetByPeer`, `TlsAlert`, `HttpConnectionClosing`, ...). No phase field: the taxonomy already names it for every kind that has one, and `TlsFailed` cannot say where it failed. No HTTP status number for `HttpStatus`; the claim is limited to concrete I/O causes. No optional metadata bag.
|
||||
- `dot_client.zig` and `doh_client.zig` implement `Leaf`; a `Transact.Failure` becomes a returned `Fault` instead of a thrown error. The DoT stale-session redial is unchanged and still counted through `reuse_recoveries`, never as a fault. The retry list is not widened; with the cause recorded, a stale-session cause outside the four lifecycle errors shows in the next episode and the list grows on evidence.
|
||||
- `Pool` consumes `Leaf` for its slots and keeps implementing `transport.Client` toward the handler, the forward client, and the fakes that sit on that side. The outer error set, `transport.group`, and the handler are untouched; the pool returns `last_fault.kind` as the error it returns today. The race harness stays generic: the pool races the leaf through a function whose error set adds `Timeout`; a leaf never returns `error.Timeout` itself (its own timeouts are faults), so `error.Timeout` out of the race is the harness. With `race == .expired and !truncated` the pool synthesizes `Fault{ .kind = error.Timeout, .cause = error.Timeout }` and records it: an untruncated attempt expiry is peer evidence. With `truncated` it stays `error.BudgetExhausted`, unattributed, exactly as today.
|
||||
- Health stores the effective fault (kind and cause names, `error_name_capacity` sized for both). Every surface that printed the taxonomy prints one text: `<Kind> (cause <Cause>)`, e.g. `SendFailed (cause BrokenPipe)`. Those surfaces are the episode detail, `upstream 'tls://1.1.1.1:853' failed: SendFailed (cause BrokenPipe)`, `Snapshot.last_error` and its one reader, the `nxdns check` FAIL line, and the pool's debug `AttemptFailure` line. `/api/health`, `/api/upstreams` and `/metrics` carry no fault text today and gain none here.
|
||||
- `transport.raceUntilTagged` becomes generic over the raced function's own error set: it returns `RacedError(f) || error{ Timeout, SystemResources, Canceled }`, so the pool's attempt wrapper returns `LeafError || error{Timeout}` with no `@errorCast` and no `unreachable`; `error.Timeout` out of the race is the harness by type, which sets `race = .expired` before returning it; the loop's catch asserts that and handles it in the expiry branch only, so a regression of the harness contract traps in ReleaseSafe instead of blaming an endpoint. The compiler enforces at the race boundary that a leaf cannot throw a `PeerFault` or `BudgetExhausted`. A leaf's own timeout is a returned `Fault{ .kind = error.Timeout, .cause = <leaf cause> }` with `race == .completed`, and a pool test proves it is recorded with the leaf's cause and never confused with the harness expiry.
|
||||
- The blocklist fetcher's `last_failure` side field stays as it is; same shape, other subsystem, its own addendum.
|
||||
|
||||
### Tests
|
||||
|
||||
- health: one failure returns `clear`; the second returns `tripped`; a success on a tripped state returns `clear`; a stale success behind a newer failure projects nothing; a stale failure returns the newer effective fault; revisions strictly increase.
|
||||
- pool: a `tripped` effect delivered after a newer `clear` effect is dropped and no episode is open afterwards; a `clear` delivered after a newer `tripped` is dropped and the episode stays open; an episode open in the store before the pool exists is resolved by boot reconciliation, one for a url the pool does not have and one for a url it has; a retired generation's late `tripped` is corrected by the reconciliation at its last release, and the idle-replace path reconciles too; a reconcile of a tripped entry with an open episode moves neither `occurrences` nor `last_seen`; a stale success while tripped projects nothing and the occurrence count does not move; one failed exchange opens no episode and the second opens one that the next success resolves; a truncated expiry records nothing and returns `BudgetExhausted`; an untruncated expiry records `Timeout (cause Timeout)`; a leaf fault of kind `Timeout` is recorded with the leaf's cause.
|
||||
- clients: the DoT integration tests assert the returned `Fault` with kind and cause, not the kind alone; a DoH integration test drives `DohClient.exchange` against a loopback port that is bound but never listens and stays bound for the test's duration, so the connect is refused deterministically with no port reuse race, and asserts the cause survives to the returned `Fault`; the stub-connection helper tests keep their causes.
|
||||
- surfaces: the `nxdns check` test asserts the `<Kind> (cause <Cause>)` text.
|
||||
|
||||
### Out of scope
|
||||
|
||||
An idle timer on the DoT session, a chronic-rate predicate, the fetcher migration, and any change to the DoT retry list.
|
||||
|
||||
Reference in New Issue
Block a user