milestone 31: concurrent upstream exchanges, dot session reuse, queue metrics

specs/milestone-31.md

@@ -0,0 +1,201 @@
+# Milestone 31: upstream concurrency
+
+Fix the field defect the Pi deployment reproduced: with one enabled upstream, 30 concurrent unique-name queries resolve as a serial staircase (~92 ms per query), because the pool serializes all exchanges per entry (`Entry.busy` held across the whole exchange, pool.zig:226-247) and the DoT client dials a fresh TCP connection and full TLS handshake per query (dot_client.zig:158-165). Under load, waiters burn the 5 s total budget in the queue and cancel into SERVFAIL, and nothing on any surface shows the queue. Three changes: N concurrent in-flight exchanges per entry, DoT session reuse across exchanges, and queue visibility on `/metrics`.
+
+Acceptance is the Pi burst repro: 30 concurrent unique names against one enabled DoT upstream must complete without SERVFAIL and without a serial staircase. That run happens on the Pi after deploy (coordinated with the `rpi-nixos-iac` session); the in-repo gates below are the hermetic equivalents.
+
+Codex design review round 1 folded in; the corrections are marked (C1..C12) where they changed the shape.
+
+## Sessions
+
+S1: per-entry slots in the pool (Zig). S2: DoT session reuse (Zig). S3: metrics surface + docs + changelog. Sequential: S1 → S2 → S3. S1 and S2 both touch `src/app.zig` and `src/cli.zig`; S2 and S3 both touch `src/upstream/pool.zig` — sequencing removes every overlap.
+
+---
+
+## Session S1: per-entry concurrency in the pool
+
+### S1.1 Slot structure
+
+`pool.zig`. Replace `Entry.client` + `Entry.busy` with:
+
+```zig
+pub const Slot = struct {
+    client: transport.Client,
+    busy: std.Io.Mutex = .init,
+};
+
+pub const Entry = struct {
+    endpoint: transport.Endpoint,
+    slots: []Slot,            // caller-owned, len >= 1
+    priority: i32,
+    enabled: bool,
+    health: health.State,
+    sem: std.Io.Semaphore,    // .{ .permits = slots.len } at build time
+    in_flight: std.atomic.Value(u32) = .init(0),
+    peak_in_flight: std.atomic.Value(u32) = .init(0),
+    queued_total: std.atomic.Value(u64) = .init(0),
+    queued_ns_total: std.atomic.Value(u64) = .init(0),
+    /// Points into `Upstreams`' stable counter storage (S1.3). Incremented by a
+    /// DoT slot client when a stale reused session was recovered by the single
+    /// redial (S2). Zero forever for DoH entries.
+    reuse_recoveries: *std.atomic.Value(u64),
+};
+```
+
+**(R2-1)** The recovery counter lives *outside* the entry: `Pool.init` sorts entries by value, so a pointer into an `Entry` taken before the sort would dangle onto a different upstream's field. `Upstreams.build` allocates a stable `recovery_counters: []std.atomic.Value(u64)` (one per enabled upstream, zero-initialized, freed in `deinit`); both the `Entry` and its DoT slot clients hold pointers into it, and the sort copies those pointers intact. `cli.probeUpstreams` points its one-entry pool at a local counter.
+
+`Pool.init` keeps its sort; sorting copies entries by value, and that stays legal because it runs before any task can reach the pool (same argument the current `busy` default makes). The semaphore's mutex/cond are plain values; assert in `init` that every entry has `slots.len >= 1` and `sem.permits == slots.len`.
+
+The compiled slot count lives here: `pub const slots_per_entry = 8;`. No config knob (anti-requirement A2). Rationale recorded in the doc comment: listeners admit up to 64 UDP in-flight + 64 TCP connections per family; 8 concurrent exchanges per upstream clears the Pi burst (30 concurrent) in ≤4 waves even before session reuse, and costs ~533 KiB of TLS buffers per DoT entry (8 × 4 × `tls.Client.min_buffer_len`, min_buffer_len = `tls.max_ciphertext_record_len`).
+
+### S1.2 Acquisition protocol
+
+In `exchangeLoopLen`, per candidate entry, replacing the `busy.lock` block:
+
+1. Read `in_flight` with `.acquire`; if `>= slots.len`, this task counts as queued: take a start timestamp (`.awake` clock) now.
+2. `try entry.sem.wait(io)` — cancelable, exactly like the old `busy.lock(io)` (`sem.wait` either decrements a permit or returns `error.Canceled`; it cannot return holding one). **(C5)** If step 1 marked this task queued, the counters land on *both* exits: on `error.Canceled`, add `queued_total += 1` and `queued_ns_total += elapsed` (plain atomic adds, nothing cancelable) *before* propagating — a waiter that burned its budget in the queue is exactly the field failure and must be counted.
+3. On successful acquisition with step 1 marked: same two adds.
+4. **(C6)** The queued detection is approximate by construction: between the releaser's `in_flight` decrement and its `sem.post` (and the mirror window on acquire) a reader can misjudge saturation. The counters are admission samples, not an exact queue length. This is stated in the `Entry` doc comment and in the metric HELP text (S3.1); no attempt at an exact count — that would put a lock around the hot path to size a household queue.
+5. Scan `slots` with `busy.tryLock()`; after a semaphore permit, at least one slot mutex is free, so the scan must succeed — `unreachable` if it does not (that is a protocol bug, not a runtime condition).
+6. `in_flight.fetchAdd(1, .acq_rel)`, `peak_in_flight.fetchMax(...)`.
+7. Run the existing attempt against `slot.client` (same `attempt()` race, same health bookkeeping, same `selected` write, same pass-0 health re-check after acquisition).
+8. Release in a defer, in this order: `busy.unlock()`, `in_flight.fetchSub(1, .acq_rel)`, `sem.post(io)`. `post` is uncancelable, so a canceled attempt still returns its permit.
+
+The pass-0 re-check keeps its current meaning: an entry that entered backoff while this task queued is skipped after acquisition (the defer releases the slot before `continue`).
+
+**(C12)** Module doc comment rewrite (pool.zig:7-46): the guarantee becomes "at most `slots.len` in-flight exchanges per entry"; the "two concurrent queries do serialize / right trade at household scale" paragraph is deleted (it is the defect); and lines 11-13 ("an outer expiry always means an attempt was in flight") are corrected — a total-budget expiry can now also mean the task died waiting in an entry's queue, which is precisely what `queued_total` records.
+
+### S1.3 Composition root fan-out
+
+`src/app.zig` (`Upstreams.build`, app.zig:1154-1270). Each enabled upstream gets `slots_per_entry` leaf clients instead of one:
+
+- DoH: `slots_per_entry` `DohClient` values per entry, each with its own `request_buf` (1024) + `transfer_buf` (4096); all share the one `std.http.Client` (its connection pool already handles concurrent requests; `DohClient`'s only mutable state is its two buffers, doh_client.zig:105-139, so one client per slot makes each slot single-flight by construction).
+- DoT: `slots_per_entry` `DotClient` values per entry, each with its own four `tls.Client.min_buffer_len` buffers, sharing `bundle`/`bundle_lock`/gpa as today. Each DoT client also receives its upstream's `*std.atomic.Value(u64)` from `recovery_counters` (R2-1, S2.1) — wired here even in S1, as a pointer parameter that S1's `DotClient` accepts and stores unused, so S2 does not touch `app.zig`'s allocation shape again.
+- **(C11)** New allocations, named: `slot_storage: []pool_mod.Slot` of `enabled * slots_per_entry`, sliced per entry into `Entry.slots` (never pointing into the client arrays), and `recovery_counters` (R2-1). **(R2-2)** `Upstreams` stores the initialized extents — `dot_used` and `doh_used` counts (malformed/skipped upstreams leave the tails of the over-allocated client arrays uninitialized) — and every teardown path iterates only `dot[0..dot_used]`. `Upstreams.deinit` order: close every *initialized* DoT client first (S2's `DotClient.close`), then free `slot_storage`, `recovery_counters`, the client arrays and the buffer blocks. `build`'s failure-path `errdefer`s use the same extents, so a half-built `Upstreams` never closes a session that was never opened nor reads an undefined `DotClient`. In S1 (no sessions yet) the close pass is a loop over the initialized extent whose body S2 fills in.
+- Client and slot arrays are never resized after wiring: S2 pins live TLS state inside `DotClient`.
+
+**(C9)** `cli.probeUpstreams` (cli.zig:935-1010) stays sequential and builds one-slot entries (`slots.len == 1`), but does not stay otherwise unchanged: once S2 lands, a successful DoT probe leaves a session open on a client the loop is about to overwrite, whose buffers the next probe reuses. The probe loop closes its `DotClient` at the end of every iteration, on success and on every error path (`defer dot.close(io)` scoped per iteration). S1 introduces the `close` call site with S1's no-op close; S2 gives it meaning.
+
+### S1.4 Tests (pool.zig)
+
+**(C10)** The `Fake` first: multiple slots wrapping one fake removes the serialization that made `calls`, `behavior` and `then` safe to mutate plainly. `calls` becomes `std.atomic.Value(usize)`; `behavior`/`then` swap moves under a `Fake`-owned `std.Io.Mutex` (or the test builds one fake per slot where the scenario allows). Existing tests keep their assertions; the counters they read change type.
+
+- Concurrent exchanges through one entry overlap: with `slots.len = 2` and a slow fake, two concurrent calls reach `peak_in_flight == 2` on the fake and finish in ~one sleep, not two. (Replaces the current "do not overlap" test, whose claim inverts: the new claim is "at most `slots.len` in flight", pinned by a third concurrent call — fake `peak_in_flight` never exceeds `slots.len`.)
+- Queue accounting: with `slots.len = 1` and a slow fake, a second concurrent call lands `queued_total == 1` and non-zero `queued_ns_total`; an uncontended call adds neither.
+- **(C5)** A queued waiter canceled by the total budget still counts: exhaust the single slot with a stalling fake, race a second call with a total budget shorter than the stall — it fails `error.Timeout` *and* `queued_total`/`queued_ns_total` advanced.
+- A canceled waiter leaves the permit count intact: after the test above, a fresh call still acquires and completes.
+- The existing suite (failover, backoff-while-queued, attribution, budgets, diagnostics episodes) passes with entries built through a test helper that wraps a fake into a 1..N-slot entry.
+
+### S1.5 Acceptance (S1)
+
+- [ ] `zig build test` green; `zig build test -Dintegration` green.
+- [ ] The S1.4 tests exist and pass.
+- [ ] `cli.zig` probe path builds, probes each upstream exactly once, and closes its DoT client every iteration.
+
+---
+
+## Session S2: DoT session reuse
+
+### S2.1 Persistent session state
+
+`dot_client.zig`. `DotClient` gains an optional open session it owns:
+
+```zig
+const Session = struct {
+    stream: net.Stream,
+    tls: tls_client.TlsStream,
+};
+session: ?Session = null,
+/// Points at the owning pool entry's counter; null in `nxdns check` probes.
+reuse_recoveries: ?*std.atomic.Value(u64),
+```
+
+**(C1)** `TlsStream` self-pins (it hands `tls.Client` pointers into its own reader/writer fields, tls_client.zig:141-147), so a `Session` must never be built locally and copied into `self.session`. Emplacement order is part of the contract: connect the socket; set `self.session = .{ .stream = <socket>, .tls = undefined }`; then run `TlsStream.init` *in place* through a pointer to the stored payload (`&self.session.?.tls` via an `if (self.session) |*s|` capture). **(R2-3)** On handshake failure the order is: extract the concrete cause from the in-place `TlsStream` fields first (`concreteHandshake` reads `stream_reader.err`/`stream_writer.err` — resetting the optional first would destroy the only storage holding a cancellation or local-resource cause), then close the raw socket, then set `self.session = null`, then log/map/return the captured cause. No partially initialized session may survive the call. The `DotClient` itself never moves after wiring (S1.3 guarantees the array is final), so the pinned pointers stay valid across exchanges.
+
+`DotClient.close(io)`: if a session is open, `transport.closeBlocked` the TLS stream then the socket, set `session = null`; tolerate an absent session. `Upstreams.deinit` and the probe loop call it (S1.3).
+
+### S2.2 Exchange protocol with reuse
+
+`exchange` becomes:
+
+1. If `session == null`: dial + handshake with the emplacement order above; failures classify exactly as today through `transport.mapPhase` **(R2-6)** — peer fault unless the concrete cause is local-resource or cancellation, which keep their groups. A session dialed by this call is *fresh* for the rest of these rules.
+2. Send the framed query, read the framed response through the stored session. **(C4)** The two-byte length prefix is read byte-at-a-time (or through an explicit received-byte count), so "no response byte received yet" is an observable predicate, not an inference from `readSliceAll`'s all-or-nothing surface — a failure after one prefix byte is *after* first byte.
+3. On success: keep the session open and return the reply. `validateResponse` failures (`BadResponse`, `ResponseMismatch`, `ResponseTooLarge`) close the session before returning — the stream position after a bad frame is untrustworthy.
+4. **(C3)** Retry-once eligibility is three conditions, all required: the session was **reused** (existed before this call); **no response byte** of this exchange was received yet; and the **concrete unwrapped cause** (via the existing `concreteRead`/`concreteWrite` helpers) is a connection-lifecycle error: `error.EndOfStream`, `error.TlsConnectionTruncated`, `error.ConnectionResetByPeer`, `error.BrokenPipe`. Then: close the session, dial fresh once, retry the exchange once; the retry's outcome is the exchange's outcome. The recovered failure is never recorded against health, never emits a diagnostics episode, and logs at debug only — an upstream closing an idle connection is normal (RFC 7858 keepalive is advisory). On a *successful* recovery, increment `reuse_recoveries` (the pointer from S2.1) — the churn is visible as a counter, not as log spam.
+5. **(C2, C3)** Every other failure is final and classifies exactly as today (local resource and cancellation propagate through `transport.mapPhase` untouched; TLS protocol faults, cert faults and post-first-byte faults are peer faults) — and every final I/O or validation failure **closes the session before returning**: after any failed send, receive or validate, framing and TLS state are untrustworthy, and the next exchange must start from a fresh dial.
+6. Cancellation (`error.Canceled` unwrapped anywhere) closes the session — mid-frame state is unknown — and propagates.
+
+The attempt budget covers the whole of `exchange` including the one redial (the pool's `raceWithin` already wraps it; on budget expiry the pool cancels the attempt, and rule 6 closes the session).
+
+The eligibility decision is a pure function — `retryDecision(reused: bool, bytes_received: bool, cause: anyerror) enum { retry, final }` or equivalent — so the matrix is table-testable without a wire.
+
+### S2.3 Tests
+
+**(C8)** Flag discipline: `-Dintegration` is hermetic loopback only; external hosts stay under the live/manual flag (`dot_client_live_test.zig`'s existing gating). The reuse and recovery *wire* claims are therefore in-repo loopback tests, not live-network tests, and they gate the milestone:
+
+- A loopback DoT fixture: the `-Dintegration` mbedTLS `tls_server` loopback (already built for `platform/tls_server.zig`) serves framed DNS responses. **(R2-4)** The fixture certificate is self-signed and `DotClient` hardcodes `.ca = .system`, so the test preloads the fixture certificate into the test's `Certificate.Bundle` and constructs the client with `tls_name` matching the fixture cert's dNSName SAN — no insecure-verification option is added to the production client (that would be out of scope and a hole). Against it: two sequential exchanges through one client complete over one connection (`session != null` after the first; the server observed one accept); a server-side close between exchanges recovers via the single redial with no health-visible fault, no diagnostics episode (assert via `events_fixture` as in pool.zig:1143), and `reuse_recoveries == 1`; **(C4, R2-5)** the one-prefix-byte case runs on a *reused* session — one successful exchange first, then the server sends exactly one prefix byte and closes — and asserts a final failure with no second accept (no redial): on the fresh path "no retry" would hold vacuously.
+- Table tests on the pure decision function: fresh-dial failure → final; reused + pre-byte + lifecycle cause → retry; retry failure → final; reused + post-byte → final; reused + pre-byte + non-lifecycle cause (e.g. `error.TlsAlert`, `error.SystemResources`, `error.Canceled`) → final; validate failure → close+final.
+- Cancellation closes the session (stub technique as `stubStream`).
+- Live tests (existing flag) may add a real-host reuse smoke but nothing in acceptance depends on them.
+
+### S2.4 Acceptance (S2)
+
+- [ ] `zig build test` and `-Dintegration` green, including the loopback reuse/recovery/one-prefix-byte tests.
+- [ ] The decision-function table tests pass.
+- [ ] No health counter and no diagnostics episode from a recovered stale-reuse failure; `reuse_recoveries` counts it instead.
+- [ ] `Upstreams.deinit` and the probe loop close open sessions (no fd leak across probe iterations — assert the probe loop's close runs on the error path too).
+
+---
+
+## Session S3: visibility, docs, changelog
+
+### S3.1 Snapshot + /metrics
+
+- `Pool.Snapshot` gains `in_flight: u32`, `slots: u32`, `queued_total: u64`, `queued_seconds_total: f64` (from `queued_ns_total`), `reuse_recoveries_total: u64` **(C7)**. `snapshot()` reads the atomics with `.acquire`; `peak_in_flight` stays test-only.
+- `src/web/metrics.zig`: extend `UpstreamSample`/`renderUpstreams` (metrics.zig:105-116, 477-512) with `nxdns_upstream_in_flight` (gauge), `nxdns_upstream_slots` (gauge), `nxdns_upstream_queued_total` (counter), `nxdns_upstream_queued_seconds_total` (counter), `nxdns_upstream_reuse_recoveries_total` (counter), same index+redacted-url labels. **(C6)** The two queued HELP strings say "approximate; sampled at admission".
+- `/api/health` upstreams condition: unchanged (anti-requirement A3).
+
+### S3.2 Docs + changelog
+
+- PLAN.md §9: concurrency paragraph updated to the slot model and DoT reuse.
+- `docs/how-to/troubleshoot.md`: a short section — SERVFAIL bursts under concurrent load, read `nxdns_upstream_queued_total`/`in_flight` for queueing and `nxdns_upstream_reuse_recoveries_total` for connection churn.
+- CHANGELOG.md Unreleased: the defect (serialized upstream exchanges, per-query DoT handshakes), the fix, the new metrics.
+- `docs/reference/api.md`: only if it documents /metrics series today — match the existing level of detail; otherwise untouched.
+
+### S3.3 Acceptance (S3)
+
+- [ ] `zig build test -Dintegration` green; `zig build -Dadmin-dist` green (no admin changes expected — the build gate only proves nothing broke).
+- [ ] A metrics test (existing metrics.zig test style) pins the five new series render with the fixture pool.
+
+---
+
+## File ownership
+
+Sequential sessions. S1: `src/upstream/pool.zig`, `src/app.zig`, `src/cli.zig`, `src/upstream/dot_client.zig` (the unused `reuse_recoveries` parameter + no-op `close` only), `src/upstream/doh_client.zig` (only if a buffer-len constant moves; behavior unchanged). S2: `src/upstream/dot_client.zig`, `src/upstream/dot_client_live_test.zig`, the loopback fixture file(s), `src/app.zig`/`src/cli.zig` (teardown call sites only). S3: `src/upstream/pool.zig` (Snapshot), `src/web/metrics.zig`, PLAN.md, docs/**, CHANGELOG.md.
+
+## Anti-requirements
+
+- A1: no upstream name resolution, no new transports, no changes to failover order or backoff semantics.
+- A2: no config knobs for concurrency — `slots_per_entry` is compiled; the listener caps stay compiled too.
+- A3: no /api/health schema change, no openapi change, no admin change.
+- A4: no TLS session tickets/resumption — `tls.Client` in 0.16.0 has no such API here; reuse means keeping the connection open, nothing more.
+- A5: no connection keepalive timers or background probes; staleness is detected at use.
+- A6: no DoH changes beyond per-slot buffer fan-out — `std.http.Client` already pools connections.
+- A7: no exact queue-length accounting — the queued counters are admission samples (S1.2 step 4).
+
+## Acceptance (milestone complete)
+
+- [ ] All three sessions' gates green.
+- [ ] Deployed to the Pi and the burst repro re-run by the `rpi-nixos-iac` session: 30 concurrent unique names, zero SERVFAIL, no serial staircase (wall clock for the burst well under 30 × single-query latency).
+- [ ] `/metrics` on the Pi shows the five new series.
+
+## Implementation notes (post-build sync)
+
+Deviations the build kept, judged defensible in review:
+
+- `Upstreams.deinit` frees in reverse-allocation order rather than the C11 listing order; the constraint that matters — initialized DoT sessions close first — holds, and nothing reads the passive allocations after that.
+- The S2.3 cancellation "stub technique" became a real loopback test: a stubbed `TlsStream` cannot survive `close` (it calls `client.end()` through an undefined `tls.Client`), so the wire test proves the claim directly. The pure table test also covers cancellation-is-final.
+- `error.BrokenPipe` in the C3 lifecycle set is unreachable from `net.Stream.Writer.Error` in 0.16.0; it stays in `retryDecision` per the contract, reached only by the table test today.
+- `Certificate.Bundle` has no in-memory PEM entry point in 0.16.0; the loopback test mirrors `addCertsFromFile`'s decode+parse calls to preload the fixture cert.
+- The S2.4 probe-close criterion is pinned as far as the repo can observe it: the failed-probe path runs the per-iteration deferred `close` (cli test, honestly named), and close-after-failure/double-close safety is pinned in the DoT integration tests. Close after a *successful* probe is unreachable in-repo (the probe verifies against the system trust store, and the only in-repo DoT peer is self-signed); no production surface was added to force it.
+- The metrics S3.3 "fixture pool" test drives a real `Pool` through the real `snapshot()` → `upstreams()` → render path with distinct per-field values.