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:

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:

Read in_flight with .acquire; if >= slots.len, this task counts as queued: take a start timestamp (.awake clock) now.
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.
On successful acquisition with step 1 marked: same two adds.
(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.
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).
in_flight.fetchAdd(1, .acq_rel), peak_in_flight.fetchMax(...).
Run the existing attempt against slot.client (same attempt() race, same health bookkeeping, same selected write, same pass-0 health re-check after acquisition).
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 errdefers 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:

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:

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.
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.
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.
(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.
(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.
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.

22 KiB Raw Permalink Blame History Unescape Escape

Milestone 31: upstream concurrency