nxdns/specs/milestone-16.md

# Milestone 16: contained behavioral fixes

Goal: fix the verified bugs and silent failures from `TECH_DEBT.md` themes 3,
5, 6 and 7 — each one localized, each with a test that fails before and
passes after. No refactors: the duplication work is milestone 18, and a
refactor that must simultaneously fix behavior is how mirrored copies diverge
further.

**PROVISIONAL.** Written before milestone 15 was built. m15 touches
`build.zig`, workflows, fuzz files, `filter_integration_test.zig`, `cli.zig`,
`docs_drift_test.zig` and `logging.zig` — re-verify line references in those
files before starting.

## Rulings (binding)

### 1. `loadSource` propagates cancellation

`src/filter/manager.zig:541-544` and `:547-550`: two identical catch sites
fold `error.Canceled` into `loadFailure(...)`, recording a bogus "Canceled"
load-failed status, consuming the one-shot cancellation, and publishing a
snapshot with the source excluded. Ten other catch sites in the file
propagate it correctly. Add `if (err == error.Canceled) return
error.Canceled;` beside the existing OutOfMemory arm at both sites —
`Manager.Error` already has the member. Unit test: a reader that fails with
Canceled during `loadSource` makes the whole call return Canceled and writes
no status.

### 2. `commitStatus` never drops an outcome silently

`src/filter/manager.zig:1237-1250`: the id-match loop falls through with no
log when a source has no status entry (reachable via a startupPass /
web-insert race; milestone-5 policy says every non-ok state is recorded and
surfaced). Add a `log.warn` after the loop naming the source id. Unit test:
committing a status for an unknown id emits the warning and does not crash.

### 3. Downloads and compiles leave the writer lock

`src/filter/manager.zig:609-624`: `refreshAll` holds `writer_lock` across
every download (300 s budget each, `app.zig:85`) and every compile; every
web mutation ends in `Manager.reload` on the same lock, so an unrelated rule
save parks uncancelably — on a request path with no timeout, occupying one
of 64 web slots — until the pass ends. The download-to-tmp flow already
exists (`download` writes `<id>.raw.tmp` via `compiledName`, with
`deleteQuietly` defers at :646-648); the lock is just taken too early.

Restructure:

- New `refresh_lock: std.Io.Mutex` on Manager, serializing refresh passes
  against each other only.
- Split `refreshOne` (:626-729): the fetch/detect/compile stages (:650,
  :659, :668) run under `refresh_lock` alone; the publish stage (:711), the
  status commit and the final `reloadLocked` run under `writer_lock`.
- `refreshAll` takes `refresh_lock` for the pass, and `writer_lock` only
  per-source around publish plus once for the final reload. `refreshSource`
  (:582-586) follows the same split.
- The lock-ordering rule, documented on both mutex fields: `refresh_lock` is
  never acquired while holding `writer_lock`.
- `refresh_lock` serializes blocklist-**directory maintenance** too, not
  just refresh passes: `pruneOrphans` (:1040, invariant comment at
  :1130-1136) today assumes every temp-file writer holds `writer_lock`;
  once downloads write `.raw.tmp`/`.list.tmp`/`.wild.tmp` under
  `refresh_lock` alone, a concurrent source deletion (blocklists.zig
  delete → reload → pruneFiles) could sweep an in-flight refresh's temp
  files. `pruneOrphans` therefore takes `refresh_lock` first (then
  `writer_lock` if it needs it — same order as everywhere), and its
  invariant comment is rewritten. Regression test: a source deletion
  during a stalled refresh must not delete the stalled refresh's temp
  files.

Integration test (`filter_integration_test.zig`): with a refresh pass parked
on a deliberately stalled fixture route, a concurrent rule mutation
completes without waiting for the pass. DNS serving was never affected and
stays covered by the existing tests.

### 4. Truncated responses are never cached

`src/cache/dns_cache.zig:106-131`: `classify` reads `rcode` and `ancount`
and never `flags.tc`, so a TC=1 answer from a misbehaving upstream is cached
for up to `max_ttl_seconds` (86 400 s) and re-served — TC bit intact, even
over TCP, which can loop retrying clients (RFC 2181 §9). Add `if
(p.header.flags.tc) return null;` (`flags.tc` is `src/dns/header.zig:20`).

`transport.validateResponse` is deliberately untouched: rejecting TC there
would change failover semantics for every exchange, and the forward client
legitimately reads TC for its UDP-to-TCP retry (`forward_client.zig:178`).
Record: considered, rejected. Unit test beside the existing classify tests:
a TC=1 NOERROR response classifies as null.

### 5. The format sniffer stops eating hosts files with `##` banners

`src/filter/parsers.zig:54-73`: `hasAbpMarker` runs before `isComment`, and
`isElementHiding` (:86-93) matches `##` (and `#@#`, `#?#`, `#$#`, `#%#`)
unanchored with no comment guard — unlike the `$` branch two lines later,
which is guarded. A hosts file with a `##` banner parses whole as ABP:
`0.0.0.0` enters the domain set and hosts lines with inline URL comments are
silently dropped (real blocking loss, reproduced with the URLhaus banner
style). The milestone-5 as-built note (specs/milestone-5.md:1677-1678)
already *claims* the separators are matched anchored — the code never was.
Guarding the branch with `!isComment` is a circular no-op (`isComment`
consults `isElementHiding`); do not attempt it.

Fix — a positional predicate in `isElementHiding`: a separator at offset `p`
counts as element hiding only when

- `p == 0` and the character after the separator is not whitespace, `#`, or
  end of line (a generic rule `##.ad` counts; a banner `## Title`, `####`,
  or bare `##` does not), or
- `p > 0` and `line[p - 1]` is not whitespace and not `#` (a rule
  `example.com##.ad` counts; prose `see ## below` does not).

Required test cases, in `parsers.zig`: the URLhaus banner style sniffs as
hosts; `##.ad-banner` sniffs as ABP; `example.com##.ad` sniffs as ABP;
`#@#exception` after a domain sniffs as ABP; a `#`-initial line containing
`##` sniffs as a comment. The fuzz target (`blocklist_fuzz.zig`) already
covers `detectFormat` for crashes.

### 6. VACUUM respects the disk monitor

`src/storage/retention.zig`: `runOnce` (:78) does prune → checkpoint →
(every 7th pass, :94-95) VACUUM, and takes no monitor — the word does not
appear in the file — while its two sibling tasks gate on
`disk_monitor.writesAllowed()` (logger.zig:386 as a parameter,
manager.zig:1058 as a field). VACUUM is the most expensive write the
program makes and fires unconditionally on the exact filesystem the monitor
watches, then fails SQLITE_FULL with no retry for seven daily passes.

`runOnce` and `run` gain `monitor: ?*disk_monitor.Monitor` (the logger's
parameter shape). Only the VACUUM step gates: prune and checkpoint keep
running — they free space. A skipped vacuum increments a new
`Stats.vacuums_gated` and retries on the *next* pass (drop the modulo-only
trigger for a `passes_since_vacuum >= vacuum_every_passes` counter that
resets on success). `app.zig:561` passes the monitor like :560 does for the
logger. Tests: extend the cadence tests (:223) with a gated pass — the
vacuum is skipped, counted, and runs on the next allowed pass.

### 7. An oversized Cookie header degrades loudly, and the session survives

`src/web/server.zig:612-620` (`copyHeader`): a header value over the buffer
(cookie buffer = `http_util.max_cookie_len` = 1024, http_util.zig:35)
returns `""` with no log — under the documented reverse-proxy-on-shared-
domain deployment (foreign cookies riding along), every request silently
401s and the operator sees an unexplained login loop.

`copyHeader` stays generic. The cookie call site (:510) changes: when the
raw header value exceeds the buffer, extract only the session pair by
running `http_util.cookieValue` (:200 — it already parses pairs and returns
a slice of the original header) with the existing session-cookie name
constant against the full value, copy that pair into the buffer as
`<name>=<value>`, and emit one `log.debug` with the dropped size (the
`.web_server` scope at :55; no secret is logged). If even the pair does not
fit, keep the empty result but still log. Integration test
(`web_integration_test.zig`): a request with a 2 KiB cookie header whose
session pair is valid is authenticated; the same header without the pair
401s.

### 8. Live view detects fatal SSE rejections

`web/src/features/live/useLiveQueries.ts`: per the WHATWG spec a non-200
response fails an EventSource permanently after one error event, so the
3-consecutive-errors threshold (:29, :125-133) is unreachable on exactly the
429/401 paths it was built for — the UI shows "Reconnecting…" forever and
the capped state and session probe are dead. `FakeEventSource` has no
readyState, so tests pass — the mocked-network class again.

- `EventSourceLike` (:9-13) gains `readyState: number`; export `const
  EVENT_SOURCE_CLOSED = 2`. The browser `EventSource` satisfies it
  structurally.
- In the error handler: `readyState === EVENT_SOURCE_CLOSED` is a permanent
  failure — close, set `capped`, run the session probe immediately,
  bypassing the counter. Transient errors (browser auto-retry pending) keep
  the existing counter path.
- `fakeEventSource.ts` gains `readyState` with the real lifecycle
  (CONNECTING → OPEN on `emit("open")`, CLOSED on `close()` and on
  `failFatal()`, a new test helper).
- New tests: a fatal rejection (single error event, readyState CLOSED)
  reaches `capped` and calls `probeSession`; a transient error still takes
  three to trip.

### 9. A timed-out DoH handshake is a handshake failure

`src/server/doh_server.zig:311-315` counts a handshake-race timeout as
`idle_timeouts`; DoT (:313-317) counts it as `tls_handshake_failures`,
which is the recorded spec ruling. Since DoH requests have no other timer,
its `idle_timeouts` metric can only ever mean handshake stalls — the same
exported name with disjoint semantics per listener. Align DoH's
`.timed_out` arm to `tls_handshake_failures`.

### 10. Idle DoH keep-alive connections are reclaimed

`src/server/doh_server.zig:70`: `idle_timeout` bounds only the handshake;
its own doc comment admits requests have none. DoH stubs hold keep-alives by
design, and with no TCP keepalive a vanished peer pins one of 64 slots until
restart — while DoT on the same LAN reclaims after 10 s.

Extend the existing race to `receiveHead` (:333), the wait for the next
request on a keep-alive connection, using the DoT out-param precedent
(readPrefix's `out_len`): a small wrapper writes the received `Request` (or
its error) through a pointer so the raced function stays `anyerror!void`.
On `.timed_out`: bump `idle_timeouts` (now truthfully named again after
ruling 9) and close the connection. Preserve the existing error triage at
:334-346 (`HttpConnectionClosing` and `ReadFailed` return uncounted; the
three structural errors count `bad_requests`/`connection_errors` as today).
The body read and `handleRequest` stay untimed, like the web listener.

New integration-gated test mirroring the DoT idle test
(dot_server.zig:1098): a client that completes the handshake and one request
then goes quiet is closed after a short idle budget; `idle_timeouts == 1`,
`tls_handshake_failures == 0`. DoH currently has no idle test at all.

### 11. SSE shutdown does not wait for a heartbeat

`src/web/sse.zig`: the Hub has no shutdown signal, so graceful drain parks
up to 15 s (`heartbeat_interval`, web/handlers/live.zig:32) per idle
subscriber — the web `beginShutdown` (web/server.zig:593-605) shuts down
sockets but nothing wakes a task inside `Hub.wait`. Measured: the SSE
integration test budgets 40 s for exactly this
(web_integration_test.zig:74-75).

- `Wake` (:33) gains `.closed`. Hub gains `closing: bool` and `pub fn
  close(self: *Hub, io: std.Io) void`: under the mutex, set the flag and
  `event.set` every active slot. `wait` returns `.closed` immediately when
  the flag is set.
- `live.zig:114`: `.closed => return`.
- `web/server.zig` `deinit` (:349): call the hub's close (via the state's
  hub pointer) immediately before `beginShutdown` (:360).
- Test: a subscriber parked in `wait` returns `.closed` promptly after
  `close`; the W10 integration teardown no longer stalls (tighten
  `sse_budget` only if the heartbeat assertion itself allows it).

### 12. The API limiter's sweep runs

`src/web/api_limiter.zig:210`: `sweep` exists, preallocates
`stale_keys` so it never allocates, is tested — and has no production
caller; only the DNS limiter got scheduled. Once 4096 distinct addresses
have been seen, the table stays full forever and every unknown-address
request pays an O(4096) eviction scan under the limiter mutex.

`runMaintenance` (app.zig:709) gains an `api_limiter: ?*ApiLimiter`
parameter, wired at :565 from the instance built at :380-388. In the loop,
beside the two existing tasks: `_ = api.sweep(io, Clock.awake.now(io));` —
note it locks itself, unlike the DNS limiter. Test: the maintenance-loop
integration coverage asserts a stale bucket disappears.

### 13. UDP/53 and TCP/53 stats reach /metrics

`src/server/udp_server.zig:38-49` and `tcp_server.zig:54-60`: both stats
structs are written and read by nothing outside their own integration
tests — `dropped_no_slot`/`dropped_oversize` have no metric, no API field,
and errors log below the default level, while the DoT/DoH siblings export
equivalent counters. The module doc sells "dropped and counted"; the count
is unobservable.

- Both gain `pub const Snapshot` + `pub fn snapshotStats` in the exact DoT
  shape (dot_server.zig:61, :232). Field names stay as-is (the
  `accepted`/`connections` unification is milestone-18 work).
- `WebState` (src/web/server.zig) gains `udp_listeners: []const
  *udp_server.UdpServer = &.{}` and `tcp_listeners: []const
  *tcp_server.TcpServer = &.{}` — S4 adds the fields with exactly these
  names and types; S2 consumes them (the app builds four listeners:
  udp6/udp4/tcp6/tcp4, app.zig:506-528).
- `metrics.zig`: sum each family across its listeners into one
  `nxdns_udp_server_*_total` / `nxdns_tcp_server_*_total` group via the
  existing `counterGroup` derivation; extend the name-assertion test
  (:722-743).
- `app.zig` wires the slices.

### 14. Forward-client counters survive the query

`src/local/forward_client.zig:44-57`: `Stats` (queries, udp_truncated,
foreign_datagrams, failures) is instrumented, documented as preventing "an
unrecorded failure mode" — and the only production caller builds a
stack-local client per query (handler.zig:394) and drops it, so the spoofing
signal does not exist.

`Handler.Stats` (handler.zig:130-154) gains `forward_udp_truncated`,
`forward_foreign_datagrams`, `forward_failures` (atomic, like the other 17;
queries are already counted by `forward_zone_answers`). After the exchange
in `viaForwardZone`, fetchAdd the client's counters into them. The metrics
reflection (`dns_stat_fields`, metrics.zig:48) picks the new fields up
without an edit — assert the three new `nxdns_dns_*_total` names in the
metrics test. `ForwardClient.Stats` itself stays plain and per-instance.

### 15. The TLS server keeps the concrete transport cause

`src/platform/tls_server.zig:418-438`: both BIO callbacks discard the
stashed cause — `net_reader.err` / `net_writer.err` (which hold
Reset/Timeout/**Canceled**, std Io/net.zig:1260/:1324) are never read
anywhere in the file — so a routine idle-budget cancel surfaces as a warn
"mbedtls_ssl_read failed" and peer resets are indistinguishable from
timeouts. The client side solved exactly this (dot_client.zig
`concreteRead`/`concreteWrite`, :303-318).

- `ServerStream` gains `recv_cause: ?anyerror = null`, `send_cause:
  ?anyerror = null`; `bioRecv`/`bioSend` stash
  `self.net_reader.err`/`self.net_writer.err` on failure before returning
  the mbedtls code.
- `ReadError` (:216-223) widens with `Canceled`; the read path
  (`readIntoBuffer`, :334) maps a stashed `error.Canceled` to it instead of
  `TlsFailed`. Callers' race harnesses already treat cancellation
  separately.
- Unit tests in the file's existing stub style: a reader whose `err` is
  Canceled yields `error.Canceled`; a Reset yields `TlsFailed` with the
  cause stashed for logging (ruling 16).

### 16. Peer misbehavior logs at debug, like the read path already rules

`src/platform/tls_server.zig`: the read path deliberately logs a peer TCP
drop at debug (:362-364, "a louder level would be a log-spam vector"), then
`close` warns on close_notify against the same dead socket (:311) and
`handshake` warns per probe (:287). `.tls_server` is not in the dedup scope
set (logging.zig:103-108), so peer-driven warns can evict genuine warnings
from the rotating log.

- `report` (:474-479) gains a level parameter. The close_notify site passes
  debug when `peer_closed` is set or the stashed cause (ruling 15) is a
  peer-class error; the handshake site likewise. Local/config failures keep
  warn.
- Add `.tls_server` to `isDedupScope` (logging.zig:103-108) and to its
  membership test (:637-644).

**Scope widened during implementation (orchestrator ruling):** the level
choice also applies to the `ssl_read` and `ssl_write` report sites, not only
the two named ones. Ruling 15's "cause stashed for logging" wording requires
the read site to consume the level, and an unconditional warn on the write
half would keep the same spam vector open through `close` → `flush` against
a dead peer. Peer-class causes are decided by pure helpers (`isPeerCause`,
`causeLevel`); `check` still passes warn.

### 17. The query log heals its own gaps

`web/src/features/queries/QueryLogPage.tsx`: the hand-rolled accumulation
(`extra`, `cursorOverride`, `generation`, :79-98) develops a silent
mid-table row gap when the base page refetches after 30 s staleness — the
newest-100 boundary moves up while `extra` starts below the old cursor, and
the stale override means load-more never heals it. On a live DNS server the
trigger is routine.

Migrate to `useInfiniteQuery`: the endpoint already maps onto it
(`getNextPageParam: (last) => last.next_before ?? undefined`; keyset
pagination confirmed server-side, handlers/queries.zig:103-114). A
background refetch then refetches all pages in order — consistent, no gap.
The three behaviors the old code carried by hand: staleness discard is
handled by the query itself; keep the `isPlaceholderData` disable on the
button; the 401 branch is already covered by the global cache-level
`handleUnauthorized` (queryClient.ts:29-30). Delete `extra`,
`cursorOverride`, `generation`, `loadingMore`, `moreError`. Rewrite the five
load-more tests; add one for the gap scenario: base refetch with new rows
between page renders leaves no discontinuity.

### 18. Password hashing leaves the config lock

`src/web/handlers/settings.zig`: `applyPut` holds `config_lock` from :286
to :341, and the argon2id call (t=2, m=19 MiB, :373-391) sits inside it at
:312 — every settings GET (:407-409 takes the same lock) and every mutation
stalls for the hash duration on the Pi 5. The login path already does it
right: copy under lock, hash unlocked, re-check generation under lock
(auth.zig:47-71), and the LiveHash generation check (auth.zig:73-79)
already closes the concurrent-install race.

Move the password-length check and the `hashPassword` call before the lock
acquisition; everything from `loadConfig` on stays inside. The hash input
is the parsed patch only, so nothing under the lock is needed. Tests: the
existing applyPut suite passes unchanged; add one that actually
distinguishes the new behavior — "both complete" already held before the
fix, the GET merely waited out the hash. A stall seam under
`builtin.is_test` (the milestone-15 rotation-seam shape) parks
`hashPassword` on an event; the test starts a password PUT, waits until
the hash is parked, completes a settings GET **while the hash is still
parked**, then releases the seam and joins the PUT.

## Sessions

S1-S5 run in parallel. One cross-session interface, fixed here: S4 adds the
`WebState.udp_listeners`/`tcp_listeners` fields exactly as ruling 13 names
them; S2 wires and consumes them without touching `web/server.zig`.

### Session S1: filter

Owns `src/filter/manager.zig`, `src/filter/parsers.zig`,
`src/filter/filter_integration_test.zig`. Rulings 1, 2, 3, 5.

### Session S2: cache, retention, counters, wiring

Owns `src/cache/dns_cache.zig`, `src/storage/retention.zig`, `src/app.zig`,
`src/server/udp_server.zig`, `src/server/tcp_server.zig`,
`src/local/forward_client.zig`, `src/server/handler.zig`,
`src/web/metrics.zig`, and the storage/server integration tests those
touch. Rulings 4, 6, 12, 13 (except the WebState fields), 14.

### Session S3: TLS listeners

Owns `src/platform/tls_server.zig`, `src/platform/logging.zig`,
`src/server/doh_server.zig`. Rulings 9, 10, 15, 16.

### Session S4: web server

Owns `src/web/server.zig`, `src/web/sse.zig`, `src/web/handlers/live.zig`,
`src/web/handlers/settings.zig`, `src/web/web_integration_test.zig`, plus
the ruling-13 field additions. Rulings 7, 11, 18.

### Session S5: frontend

Owns `web/src/features/live/*`, `web/src/features/queries/*`. Rulings 8,
17.

### Orchestrator

Strikes the closed findings in `TECH_DEBT.md`; updates the stale
milestone-5 as-built line if ruling 5 changed its truth value.

## Acceptance (milestone complete)

- [ ] Both `loadSource` catch sites propagate Canceled; the new unit test
      passes; no status row ever reads "Canceled".
- [ ] `commitStatus` warns on an unknown id (test).
- [ ] With a stalled download in flight, a concurrent rule mutation
      completes (integration test); the lock-ordering comment exists on
      both mutexes; a source deletion during a stalled refresh leaves the
      refresh's temp files alone (regression test).
- [ ] A TC=1 response classifies as null (test); `validateResponse` is
      untouched.
- [ ] The five sniffer cases pass; the URLhaus banner file parses as hosts.
- [ ] A gated pass skips only the vacuum, counts `vacuums_gated`, and
      vacuums on the next allowed pass (test).
- [ ] The 2 KiB-cookie tests pass: session extracted, non-session 401s,
      debug line emitted.
- [ ] A fatal SSE rejection reaches `capped` and probes the session
      (frontend test); a transient error still takes three.
- [ ] DoH `.timed_out` handshake counts `tls_handshake_failures`; the
      metrics name test still passes.
- [ ] The new DoH idle test passes: quiet keep-alive closed,
      `idle_timeouts == 1`.
- [ ] `Hub.close` wakes a parked subscriber promptly (test); web `deinit`
      calls it before `beginShutdown`.
- [ ] The API limiter sweep runs in maintenance (test).
- [ ] `/metrics` carries `nxdns_udp_server_*` and `nxdns_tcp_server_*`
      families summed over four listeners (name test extended).
- [ ] `/metrics` carries the three new `nxdns_dns_forward_*` counters.
- [ ] The TLS-server cause tests pass: Canceled surfaces as Canceled, never
      as a warn line; close_notify after a peer drop logs debug;
      `.tls_server` is in the dedup set.
- [ ] The query log uses `useInfiniteQuery`; the gap-scenario test passes;
      the accumulation state is gone.
- [ ] Hashing runs before `config_lock`; the settings suite passes.
- [ ] Full suite green: `zig build test -Dintegration`, `npm run test`,
      `npm run typecheck`.

## Anti-requirements

- No listener-core extraction, no shared repo helpers, no shared transport
  helpers — milestone 18.
- No stats field renames (`accepted` vs `connections`) — milestone 18.
- No config key renames and no trusted-proxy setting — milestone 17.
- No `useInfiniteQuery` migration anywhere but the query log.
- No new metrics beyond the families this spec names.
- No timeout on DoH request bodies or handlers — only `receiveHead` races.